1141415 - no expiretype or expiretime in parameter of SpecialPowers.addPermission/pushPermissions

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[C.M.Chang[:chunmin]]

nsIPermission can be added by given uri, type and some expire information[1, 2]. However, the implementation now[3] has no parameter to set expiretype and expiretime.

Usage:
1) Install an app A with permission P and set the permission expire_type to EXPIRE_SESSION[4]
2) Allocate an iframe Z and load app A into it
    -> This will add the appId's reference count
3) Check the permission P with app A
    -> A has permission P
4) Deallocate the iframe Z
    -> This will release the appId's reference count
5) Check the permission P with app A again
    -> A does **NOT** has permission P

Currently, the SpecialPowers.addPermission or SpecialPowers.pushPermissions can't set the expire_type[1] and expire_time[2]. The expire_type[1] and expire_time[2] passed to nsIPermissionManager's addFromPrincipal[5] will be EXPIRE_NEVER[6] and 0. The nsIPermissionManager's addFromPrincipal is called by SpecialPowersObserverAPI.js[7]. Thus, the appId's reference count won't be released after the frame that load the app with the permission is destroyed[8].

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIPermissionManager.idl#83
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIPermissionManager.idl#87
[3] https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#1764
[4] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIPermissionManager.idl#62
[5] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIPermissionManager.idl#103
[6] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIPermissionManager.idl#61
[7] https://dxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/SpecialPowersObserverAPI.js#322
[8] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#1372

Comment 1

[C.M.Chang[:chunmin]]

Attachment: [v1] Add parameters expire_type and expire_time to SpecialPowers.addPermission/pushPermissions

Comment 2

[C.M.Chang[:chunmin]]

Hi, Ted,
It would be nice if you can review for this patch. Thanks a lot.

Comment 3

[Martijn Wargers (dead)]

Perhaps you could add a test to this feature in http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPermissions.html?force=1 ?

Comment 4

[C.M.Chang[:chunmin]]

Attachment: testcase

Comment 5

[C.M.Chang[:chunmin]]

Hi, Martijn Wargers,
Yor're right. I already add a test for this modification. Thanks for reminding me.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8575068 [details] [diff] [review]
[v1] Add parameters expire_type and expire_time to SpecialPowers.addPermission/pushPermissions

Review of attachment 8575068 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/specialpowers/content/specialpowersAPI.js
@@ +803,2 @@
>            todo.op = 'remove';
> +        } else if(permission.hasOwnProperty('expireType') && permission.hasOwnProperty('expireTime')) {

Is there a reason you need to use hasOwnProperty here?

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8584256 [details] [diff] [review]
testcase

Review of attachment 8584256 [details] [diff] [review]:
-----------------------------------------------------------------

Just fold these two patches together for landing, please.

Comment 8

[C.M.Chang[:chunmin]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #6)
> Comment on attachment 8575068 [details] [diff] [review]
> [v1] Add parameters expire_type and expire_time to
> SpecialPowers.addPermission/pushPermissions
> 
> Review of attachment 8575068 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/specialpowers/content/specialpowersAPI.js
> @@ +803,2 @@
> >            todo.op = 'remove';
> > +        } else if(permission.hasOwnProperty('expireType') && permission.hasOwnProperty('expireTime')) {
> 
> Is there a reason you need to use hasOwnProperty here?

I just want to check whether the permission have this two properties or not, but I think typeof permission.expire* === "number" may be more explicit.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> Comment on attachment 8584256 [details] [diff] [review]
> testcase
> 
> Review of attachment 8584256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just fold these two patches together for landing, please.
Sure

Comment 9

[C.M.Chang[:chunmin]]

Attachment: add expire setting of permissions to SpecialPowers

Comment 10

[C.M.Chang[:chunmin]]

Attachment: add expire setting of permission to SpecialPowers

Comment 11

[C.M.Chang[:chunmin]]

Attachment: add expire setting of permission to SpecialPowers

try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef1882ffe4a9

Comment 12

[C.M.Chang[:chunmin]]

Comment on attachment 8587275 [details] [diff] [review]
add expire setting of permission to SpecialPowers

Hi, Ted,
I made some changes to the patch. Could you review the patch again? Thx

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 8587275 [details] [diff] [review]
add expire setting of permission to SpecialPowers

Review of attachment 8587275 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPermissions.html
@@ +19,5 @@
>  
> +const EXPIRE_TIME = SpecialPowers.Ci.nsIPermissionManager.EXPIRE_TIME;
> +var start;
> +const DELAY = 500;
> +SimpleTest.requestFlakyTimeout("untriaged");

This...isn't right. I'm not sure what we ought to be doing in this situation.

Comment 14

[(not currently active) Ted Mielczarek]

Ehsan: this test wants to test that permissions have expired at a certain time. Is there a non-flaky way to test that?

Comment 15

[(no longer active)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> Ehsan: this test wants to test that permissions have expired at a certain
> time. Is there a non-flaky way to test that?

Yes, and I think the usage of timeouts is OK here, but this patch is not doing things the right way.  The permission manager already knows how to expire stuff, as tested by test_permmanager_expiration.js.  Why do we need to do anything extra in SpecialPowers?

Comment 16

[C.M.Chang[:chunmin]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #14)
> > Ehsan: this test wants to test that permissions have expired at a certain
> > time. Is there a non-flaky way to test that?
> 
> Yes, and I think the usage of timeouts is OK here, but this patch is not
> doing things the right way.  The permission manager already knows how to
> expire stuff, as tested by test_permmanager_expiration.js.  Why do we need
> to do anything extra in SpecialPowers?

This patch want to set the expire_type or expire_time to permission_manager in SpecialPowers. Don't we need to add a test to check whether it works as expected in SpecialPowers?

Comment 17

[(no longer active)]

OK, in that case just call SimpleTest.requestFlakyTimeout and explain the reason why you're using the timeout.

Comment 18

[C.M.Chang[:chunmin]]

Attachment: Add expire setting of permissions to SpecialPowers

Hi, Joel,
I might need your help again! I modify small piece of code in specialpowers to enable expire setting in permission depending on bug 1149868. Could you review this patch when you're available?

Comment 19

[C.M.Chang[:chunmin]]

Attachment: 0001-bug-114145-Add-expire-setting-of-permissions-to-Spec.patch

Correct the typo and carry r?

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8609243 [details] [diff] [review]
0001-bug-114145-Add-expire-setting-of-permissions-to-Spec.patch

Review of attachment 8609243 [details] [diff] [review]:
-----------------------------------------------------------------

there are 3 spots in the code that we check the type of expireType, expireTime and the value of expireType.  I see you reuse one check in a few places, is there a way to reduce this at all?  For example in specialpowersAPI.js we have this common pattern:
if (check_values) {
  set_values();
}

could we just have null values and set them by default; then when we are copying or creating objects to do action on, we will copy over the expire info and not need all these clauses?

Honestly I could be wrong, but thing there is something missing here.

going with a r- for now, mostly for the test cases that could use more stuff.  Thanks for fixing these issues!

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPermissions.html
@@ +162,5 @@
> +    }], function() {
> +      testExpiredPermission(SimpleTest.finish);
> +    }
> +  );
> +}

can we test for invalid values here?  what about other values for expireType?

::: testing/specialpowers/content/specialpowersAPI.js
@@ +813,2 @@
>          var todo = {'op': 'add', 'type': permission.type, 'permission': perm, 'value': perm, 'url': url, 'appId': appId, 'isInBrowserElement': isInBrowserElement};
> +        if (permission.remove == true){

please add a space between "){"

@@ +815,2 @@
>            todo.op = 'remove';
> +        } else if(setExpire) {

please add a space between if and (

Comment 21

[C.M.Chang[:chunmin]]

Attachment: [v2] Add expire setting of permission to SpecilaPowers

Hi, Joel,
Thanks for your review again! 
You're right, it's better to use default value and modify it when necessary. In this patch, I take your advice to set default value when todo object is created.

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8610328 [details] [diff] [review]
[v2] Add expire setting of permission to SpecilaPowers

Review of attachment 8610328 [details] [diff] [review]:
-----------------------------------------------------------------

just one little nit, thanks for reworking this.  Feel free to fix this and carryover the r+.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +1900,5 @@
>        'url': url,
>        'appId': appId,
> +      'isInBrowserElement': isInBrowserElement,
> +      'expireType': (typeof expireType === "number")? expireType : 0,
> +      'expireTime': (typeof expireTime === "number")? expireTime : 0

nit: in all cases of the trinary operator please add a space ' ' before the ? character.

Comment 23

[C.M.Chang[:chunmin]]

Attachment: Add expire setting of permissions to SpecialPowers r=jmaher

- add a space ' ' before the ? in trinary operator
- carry r+

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/020ac30e270a

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for Windows mochitest-5 bustage in https://hg.mozilla.org/integration/mozilla-inbound/rev/ad064f98011b

https://treeherder.mozilla.org/logviewer.html#?job_id=10193541&repo=mozilla-inbound

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #25)
> I had to back this out for Windows mochitest-5 bustage in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ad064f98011b
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=10193541&repo=mozilla-
> inbound

Excitingly not complete permafail, one of the three retriggers came back green.

Comment 27

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/8707d35414f4

Comment 28

[C.M.Chang[:chunmin]]

From my last try[1], it can come back to green after retriggering them. I think it's timing issue because there is a strict time limit in the test. Does it only happen in win XP 32bit?
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1d74355b555

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

it appears to be a windows xp/7 thing, although the sheriffs could tell more.  In the try push referenced above the winxp mochitest-5 run had 2 oranges and 1 green.  Here is a link to the push where the error was first seen:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ed98e1b9168d

note, this is the 1st run on xp/7 since the patch had landed (the other builds had failures).

Comment 30

[C.M.Chang[:chunmin]]

Attachment: [v3] Add expire setting of permission to SpecilaPowers

Hi, Joel,
I think the problem might be caused by setTimeout(). To get rid of it, I try to use Services.obs.addObserver(handler(), 'perm-changed', false) to listen the perm-changed signals. It works for receiving permission-added signal but not for getting permission-deleted one. The reason is that the expired permission won't be cleared after timeout until other try to access it[1]. Thus, if we want to listen the permission-deleted signal by addObserver, we need to keep calling SpecialPowers.testPermission(...)[2](or other function that can operate permission), or the expired permission won't be removed. However, to keep calling SpecialPowers.testPermission(...), it still need to use setTimeout(). There're two proposals to fix the problem encountered on tryserver

proposal 1
----------------------
Simply use setTimeout to test permission only and add a time buffer to avoid the timing issues. For example:

function testExpiredPermission(callback) {
  var now = Number(Date.now());
  if (now < start + DELAY) {
    ok(SpecialPowers.testPermission('pEXPIRE', ALLOW_ACTION, document), 
       'unexpired permission is still there');
    setTimeout(function() {testExpiredPermission(callback)}, DELAY);
    return;
  }

  if (now > start + DELAY + BUFFER) {
   ok(false, "expired permission is not cleared after timeout!");
   return;
  }

  if (SpecialPowers.testPermission('pEXPIRE', ALLOW_ACTION, document)) {
    setTimeout(this._pollingCheck, DELAY);
    return;
  }

  callback();
}


proposal 2
----------------------
To know the permission changes exactly, Services.obs.addObserver should be used to monitor the permission's status with proposal 1. It might be helpful to provide more information for debugging.

Which one do you prefer? Or any better ideas? 
I temporarily take proposal 2 in this patch, but I can change it if it need.

[1] https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1372
[2] https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1333

Comment 31

[C.M.Chang[:chunmin]]

Attachment: [v3.1] Add expire setting of permission to SpecilaPowers

(In reply to Chun-Min Chang[:chunmin] from comment #30)
The buffer might be hard to be determined. It may be better to keep check permission until timeout?

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8612785 [details] [diff] [review]
[v3.1] Add expire setting of permission to SpecilaPowers

Review of attachment 8612785 [details] [diff] [review]:
-----------------------------------------------------------------

overall this is safer, I really want to understand how we handle error conditions, etc.

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPermissions.html
@@ +154,5 @@
> +}
> +
> +function test8() {
> +  afterPermissionChanged('pEXPIRE', 'deleted', SimpleTest.finish);
> +  afterPermissionChanged('pEXPIRE', 'added', permissionPollingCheck);

is there any chance that we could call finish and then have the permissionPollingCheck still running?  it seems as though I either don't understand this or we could have some issues with state and timing.

@@ +174,5 @@
> +  gScript.addMessageListener('perm-changed', function onChange(msg) {
> +    if (msg.type == type && msg.op == op) {
> +      gScript.removeMessageListener('perm-changed', onChange);
> +      callback();
> +    }

what if we never get a perm-changed message?

Comment 33

[C.M.Chang[:chunmin]]

(In reply to Joel Maher (:jmaher) from comment #32)
> Comment on attachment 8612785 [details] [diff] [review]
> [v3.1] Add expire setting of permission to SpecilaPowers
> 
> Review of attachment 8612785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> overall this is safer, I really want to understand how we handle error
> conditions, etc.
> 
> :::
> testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPermissions.html
> @@ +154,5 @@
> > +}
> > +
> > +function test8() {
> > +  afterPermissionChanged('pEXPIRE', 'deleted', SimpleTest.finish);
> > +  afterPermissionChanged('pEXPIRE', 'added', permissionPollingCheck);
> 
> is there any chance that we could call finish and then have the
> permissionPollingCheck still running?  it seems as though I either don't
> understand this or we could have some issues with state and timing.
Maybe I didn't say it clearly, sorry.

For the question, I think the answer is no. The expired permission will be removed when GetPermissionHashKey(...) in nsPermissionManager is called[1,2] after the expire time(call testPermission() after timeout is one way to trigger it). Thus, when permissionPollingCheck() is called after the expired time, the SpecialPowers.testPermission will return false and stop calling next permissionPollingCheck().


                                         --> time point X:
                                        /   The expired permission will be removed
                                       /    and a perm-changed signal with data=deleted
start                       expire    /     will be fired at this moment 
 |                             |     /
 +-----------------------------+----x-----------> Time
    ^               ^               ^
    |               |               |  
testPermission  testPermission  testPermission
                                =====================
                                SpecialPowers.testPermission return false
                                and stop calling next permissionPollingCheck()
    
The above figure illustrate the normal situation. However, I reckon it will be a little different in win XP 32-bit. The problem faced in comment 25 is caused by the time difference between javascript time and system time.

                                       --> time point X:
                                      /    In javascript, the time is expired!
                                     /     However, it's not in system!
start                       expire  /      
 |                             |   /
 +-----------------------------+--x-----------> Time[javascript]
                                  |
 |<....>|                      |<.|..>|
  time diff                       | time diff
                                  |
        +-------------------------o---+--------------> Time[system]
        |                             |
       start                       expire

In win XP 32-bit, if we call testPermission() in javascript at time point X, it will return true(we expect false here). This is why the attachment 8611124 [details] [diff] [review] fail in test. Our test is based on the javascript time' view, while the nsPermissionManager who is in charge of removing expired permission is based on system time's view. 

If |time diff| > |time x - expire time|, SpecialPowers.testPermission() will return true at time point X because nsPermissionManager doesn't think the permission is expired[3]. The permission is expired from javascript time's view actually. Nevertheless, it's not from system time's view.

One simple way to avoid this uncertain timing issue is to keep calling SpecialPowers.testPermission(in permissionPollingCheck()) until the permission is removed.

                                      --> keep calling SpecialPowers.testPermission
                                     /    until the permission is removed
start                       expire  /
 |                             |   /
 +----------o----------o-------+--x----------x-----> Time[javascript]
            |          |          |          |
 |<....>|   |          |          |          |
  time diff |          |          |          |
            |          |          |          |
        +---o----------o----------o---+------x------------> Time[system]
        |                             |
       start                       expire


function permissionPollingCheck() {
  var now = Number(Date.now());
  if (now < start + DELAY) {
    ok(SpecialPowers.testPermission('pEXPIRE', ALLOW_ACTION, document),
      'unexpired permission is still there');
    setTimeout(permissionPollingCheck, DELAY);
    return;
  }
	
  // The permission should be expired and removed after now >= start + DELAY.
  // That is, we expect SpecialPowers.testPermission below return false here.
  // However, if it return true, then it may has timing issue here.
  if (SpecialPowers.testPermission('pEXPIRE', ALLOW_ACTION, document)) {
    // Keep calling SpecialPowers.testPermission() until the permission is removed
    setTimeout(permissionPollingCheck, DELAY);
    return;
  }
}


[1] https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1372
[2] https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsPermissionManager%3A%3AGetPermissionHashKey%28const+class+nsACString_internal+%26%2C+uint32_t%2C+_Bool%2C+uint32_t%2C+_Bool%29%22
[3] https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1375

> @@ +174,5 @@
> > +  gScript.addMessageListener('perm-changed', function onChange(msg) {
> > +    if (msg.type == type && msg.op == op) {
> > +      gScript.removeMessageListener('perm-changed', onChange);
> > +      callback();
> > +    }
> 
> what if we never get a perm-changed message?
It may happen if nsPermissionManager is broken. How about using a 'count' to limit the polling times?

function permissionPollingCheck(count) {
  var now = Number(Date.now());
  if (now < start + DELAY) {
    ok(SpecialPowers.testPermission('pEXPIRE', ALLOW_ACTION, document),
      'unexpired permission is still there');
    setTimeout(function() {permissionPollingCheck(0)}, DELAY);
    return;
  }

  if (count > 10) {
    ok(false, 'expired permission should be removed!');
    SimpleTest.finish();
    return;
  }

  if (SpecialPowers.testPermission('pEXPIRE', ALLOW_ACTION, document)) {
    setTimeout(function() {permissionPollingCheck(count+1)}, DELAY);
    return;
  }
}

Comment 34

[C.M.Chang[:chunmin]]

(In reply to Chun-Min Chang[:chunmin] from comment #33)
>   if (count > 10) {
>     ok(false, 'expired permission should be removed!');
>     SimpleTest.finish();
>     return;
>   }
From my test[1], count = 2 maybe enough.
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=75d594f08e20

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for the explanation of the problem!  adding a count on checks seems like an adequate safeguard to me.  It is unfortunate that we cannot get reliable observer notifications for expired permissions.  Maybe we could add that in?

Comment 36

[C.M.Chang[:chunmin]]

(In reply to Joel Maher (:jmaher) from comment #35)
> thanks for the explanation of the problem!  adding a count on checks seems
> like an adequate safeguard to me.  It is unfortunate that we cannot get
> reliable observer notifications for expired permissions.  Maybe we could add
> that in?
If we make nsPermissionManager clear the expired permissions itself instead of removing it when others try to get the permissions, we may need to open threads to do this job. This will increase the system's workload.

On the other hand, if we can get the |time diff| between javascript time and system time, then we can know when we should call SpecialPowers.testPermission(...) and avoid this timing issue.

Comment 37

[C.M.Chang[:chunmin]]

(In reply to Chun-Min Chang[:chunmin] from comment #36)
> On the other hand, if we can get the |time diff| between javascript time and
> system time, then we can know when we should call
> SpecialPowers.testPermission(...) and avoid this timing issue.
Is it a bug that the time difference between javascript time and system time is too large in win32 XP? Make an api to calculate the time difference may be not a good idea because only win32 XP need it.

Comment 38

[Joel Maher ( :jmaher ) (UTC -8)]

do you know the time difference on winxp?  is this something large enough to account for a timezone issue?  Maybe we are lacking precision?

Comment 39

[C.M.Chang[:chunmin]]

Back to the original question mentioned in comment #32
> what if we never get a perm-changed message?
This only happen if nsPermissionManager is broken. Is it ok to use a timeout mechanism to prevent this situation?

Or... Is it possible to use 'skip-if' to skip this test on win32 XP platform? Windows XP support has ended by Microsoft already, maybe we can skip it if the test code still works fine in other platform?

Comment 40

[C.M.Chang[:chunmin]]

[1] provides another work around: set an expired permission with 100ms but test it after 200ms.
If we take the same way, the code might be like:

function permissionPollingCheck() {
  var now = Number(Date.now());
  if (now < start + DELAY) {
    ok(SpecialPowers.testPermission('pEXPIRE', ALLOW_ACTION, document),
      'unexpired permission is still there');
    setTimeout(permissionPollingCheck, DELAY + **BUFFER**);
    return;
  }
	
  ok(!SpecialPowers.testPermission('pEXPIRE', ALLOW_ACTION, document),
      'expired permission should be removed');
}

[1] https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/test/unit/test_permmanager_expiration.js#65

Comment 41

[Joel Maher ( :jmaher ) (UTC -8)]

there is no easy way to ignore winxp via a manifest, it would ignore winxp and win7.  I think we have to accept whatever solution we have.  Please ensure we have great comments in the test files so folks can see why this exists.

Comment 42

[(not currently active) Ted Mielczarek]

You can use os_version in skip-if:
https://dxr.mozilla.org/mozilla-central/source/dom/media/test/mochitest.ini#388

That '5.1' is WinXP.

Comment 43

[Joel Maher ( :jmaher ) (UTC -8)]

oh ted!  great.

Comment 44

[C.M.Chang[:chunmin]]

I found that the PR_Now() used in nsPermissionManager[1,2] is smaller than the time value got by Date.now()[3,4] on Windows(15~17ms actually). It's the root cause of this timing issue. The permission's expiration is set by Date.now(), therefore, the valid permission period in system is longer than expectation. 


  Example
=======================
Suppose that the time value got by Date.now()[javascript level] has 15ms more than PR_Now()[system level]. When we want to add a permission with expiration, we call Date.now() to get the time and used it as base to set the expireTime. If Date.now() returns 200, then it means the system time is 185. The expected permission period is 500, so the expected timeout is 200+500=700. After timeout, at time 706, the return value of testPermission() called will be 'true' against the expected 'false' because the permission is checked by system time whose value is 706-15=691 < 700. That's why the test failed in Windows.

                                        
 |<........... valid period ............>|                                         
185                           685   691 700
 |                             |     |   |
 +-----------------------------+-----+---+------> Time[system]
 .                             .     .
 .                             .     .
 .                             .     .
 +-----------------------------+-----+----------> Time[javascript]
 |                             |     |
start                        expire testPermission
 200                          700   706
  |<...... valid period ......>|


[1] https://dxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#1375
[2] https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/windows/ntmisc.c#317
[3] https://dxr.mozilla.org/mozilla-central/source/js/src/jsdate.cpp#1207
[4] https://dxr.mozilla.org/mozilla-central/source/js/src/prmjtime.cpp#154

Comment 45

[C.M.Chang[:chunmin]]

Attachment: [v3.2] Add expire setting of permission to SpecilaPowers

Unfortunately, the same problem sometimes happen on win7[1].

To skip the timing issue mentioned in comment 44 on winXP and win7, I'd like to use "skip-if = os == 'win' && (os_version == '5.1' || os_version == '6.1')" in mochitest.ini.(from [2], I guess the os_version == '6.1' is win7)

Is that OK, Joel?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6978b672f09f
[2] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/test/webgl-conformance/mochi-single.html#69

Comment 46

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8615209 [details] [diff] [review]
[v3.2] Add expire setting of permission to SpecilaPowers

Review of attachment 8615209 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mochitest/tests/Harness_sanity/mochitest.ini
@@ +11,5 @@
>  [test_SpecialPowersExtension.html]
>  [test_SpecialPowersExtension2.html]
>  support-files = file_SpecialPowersFrame1.html
>  [test_SpecialPowersPushPermissions.html]
> +skip-if = os == 'win' && (os_version == '5.1' || os_version == '6.1') # Bug ????? - PR_Now() and Date.now() are out of sync on win32 XP/win7

I want a bug on file here so we have a clear reference:)

Comment 47

[C.M.Chang[:chunmin]]

(In reply to Joel Maher (:jmaher) from comment #46)
> I want a bug on file here so we have a clear reference:)
Joel, really thanks for the review :)

I file a bug 1171443 to point out that PR_Now() and Date.now() are out of sync on win32 XP/win7. Sadly, the discussion mention that there is no way to avoid this skew. To make sure the time base is same, one way is to add a readonly attribute named systemTimestamp in [1] and use it to get system time instead of Date.now(). Nevertheless, it's unnatural to use systemTimestamp instead of Date.now().

I am really sorry to bother you with same issue multiple times.

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/system/nsIXULRuntime.idl

Comment 48

[C.M.Chang[:chunmin]]

Attachment: [plan B] Add expire setting of permission to SpecilaPowers and add a systemTimestamp to nsIXULRuntime

Implementation of comment 47

Comment 49

[C.M.Chang[:chunmin]]

Attachment: 0001-bug-11411415-add-expire-permission-setting.patch

If the fact that PR_Now() and Date.now() are out of sync can't be avoided, maybe we can handle it as a special case in code rather than use 'skip-if' to hide this. Another way is to create a new api:systemTimestamp mentioned in comment 47 instead of Date.now() and use it when we ask a permission with expire time.

I prefer to keep using Date.now() because the code will be more natural. It also leaves an example to remind developers about the timing issue on win32 platform. However, create an api to get system timestamp gives a platform-independent solution.

Again, very sorry to bother you with the same issue. I really want to find the best way to deal with this bug.

Comment 50

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8616702 [details] [diff] [review]
0001-bug-11411415-add-expire-permission-setting.patch

Review of attachment 8616702 [details] [diff] [review]:
-----------------------------------------------------------------

2 things I would like to see addressed- otherwise this looks good.

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPermissions.html
@@ +19,5 @@
>  
> +const EXPIRE_TIME = SpecialPowers.Ci.nsIPermissionManager.EXPIRE_TIME;
> +var start;
> +const DELAY = 500;
> +const TIME_SKEW = 30;

are we guaranteed that 30ms is enough for the time skew?  If we believe it is 30, lets make the value 100 and have a comment indicating where we derived time_skew from.

::: toolkit/xre/nsAppRunner.cpp
@@ +957,5 @@
> +{
> +  NS_ENSURE_ARG_POINTER(aResult);
> +  *aResult = PR_Now();
> +  return NS_OK;
> +}

do we use this at all?

Comment 51

[C.M.Chang[:chunmin]]

Attachment: [v4] Add expire setting of permission to SpecilaPowers

(In reply to Joel Maher (:jmaher) from comment #50)
> Comment on attachment 8616702 [details] [diff] [review]
> 0001-bug-11411415-add-expire-permission-setting.patch
> 
> Review of attachment 8616702 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 2 things I would like to see addressed- otherwise this looks good.
> 
> :::
> testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPermissions.html
> @@ +19,5 @@
> >  
> > +const EXPIRE_TIME = SpecialPowers.Ci.nsIPermissionManager.EXPIRE_TIME;
> > +var start;
> > +const DELAY = 500;
> > +const TIME_SKEW = 30;
> 
> are we guaranteed that 30ms is enough for the time skew?  If we believe it
> is 30, lets make the value 100 and have a comment indicating where we
> derived time_skew from.
From my test, the time diff is about 15~20ms. I leave a comment and change this buffer to 100m now!

> ::: toolkit/xre/nsAppRunner.cpp
> @@ +957,5 @@
> > +{
> > +  NS_ENSURE_ARG_POINTER(aResult);
> > +  *aResult = PR_Now();
> > +  return NS_OK;
> > +}
> 
> do we use this at all?
My bad, I forget to undo this diff. We don't need to use this!

Comment 52

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8620821 [details] [diff] [review]
[v4] Add expire setting of permission to SpecilaPowers

Review of attachment 8620821 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

::: testing/mochitest/tests/Harness_sanity/test_SpecialPowersPushPermissions.html
@@ +18,5 @@
>  const ACCESS_LIMIT_THIRD_PARTY = SpecialPowers.Ci.nsICookiePermission.ACCESS_LIMIT_THIRD_PARTY;
> +const EXPIRE_TIME = SpecialPowers.Ci.nsIPermissionManager.EXPIRE_TIME;
> +var start;
> +const DELAY = 500;
> +const TIME_SKEW = 100;

I wish we would document the time_skew here instead of line 195.

Comment 53

[C.M.Chang[:chunmin]]

Attachment: [v4.1] Add expire setting of permission to SpecilaPowers r=jmaher

move comment to TIME_SKEW and carry r+

Comment 54

[C.M.Chang[:chunmin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fed935e708dc

Comment 55

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc4bddc95462

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc4bddc95462