If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 41

Status

Testing
Mochitest
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: chunmin, Assigned: chunmin)

Tracking

(Depends on: 1 bug)

unspecified
mozilla41
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 15 obsolete attachments)

10.47 KB, patch
chunmin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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
(Assignee)

Updated

3 years ago
Assignee: nobody → cchang
(Assignee)

Comment 1

3 years ago
Created attachment 8575068 [details] [diff] [review]
[v1] Add parameters expire_type and expire_time to SpecialPowers.addPermission/pushPermissions
(Assignee)

Updated

3 years ago
Blocks: 1114507
(Assignee)

Updated

3 years ago
Attachment #8575068 - Flags: review?(ted)
(Assignee)

Comment 2

3 years ago
Hi, Ted,
It would be nice if you can review for this patch. Thanks a lot.
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 ?
(Assignee)

Comment 4

3 years ago
Created attachment 8584256 [details] [diff] [review]
testcase
(Assignee)

Comment 5

3 years ago
Hi, Martijn Wargers,
Yor're right. I already add a test for this modification. Thanks for reminding me.
(Assignee)

Updated

3 years ago
Attachment #8584256 - Flags: review?(ted)
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?
Attachment #8575068 - Flags: review?(ted) → review+
Comment on attachment 8584256 [details] [diff] [review]
testcase

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

Just fold these two patches together for landing, please.
Attachment #8584256 - Flags: review?(ted) → review+
(Assignee)

Comment 8

3 years ago
(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
(Assignee)

Comment 9

3 years ago
Created attachment 8585274 [details] [diff] [review]
add expire setting of permissions to SpecialPowers
Attachment #8575068 - Attachment is obsolete: true
Attachment #8584256 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
Created attachment 8585925 [details] [diff] [review]
add expire setting of permission to SpecialPowers
Attachment #8585274 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
Created attachment 8587275 [details] [diff] [review]
add expire setting of permission to SpecialPowers

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

Updated

3 years ago
Attachment #8585925 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
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
Attachment #8587275 - Flags: review?(ted)
(Assignee)

Updated

3 years ago
Flags: needinfo?(ted)
Flags: needinfo?(ted)
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.
Attachment #8587275 - Flags: review?(ted)
Ehsan: this test wants to test that permissions have expired at a certain time. Is there a non-flaky way to test that?
Flags: needinfo?(ehsan)
(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?
Flags: needinfo?(ehsan)
(Assignee)

Comment 16

3 years ago
(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?
OK, in that case just call SimpleTest.requestFlakyTimeout and explain the reason why you're using the timeout.
(Assignee)

Updated

2 years ago
Depends on: 1149868
(Assignee)

Comment 18

2 years ago
Created attachment 8609201 [details] [diff] [review]
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?
Attachment #8587275 - Attachment is obsolete: true
Attachment #8609201 - Flags: review?(jmaher)
(Assignee)

Comment 19

2 years ago
Created attachment 8609243 [details] [diff] [review]
0001-bug-114145-Add-expire-setting-of-permissions-to-Spec.patch

Correct the typo and carry r?
Attachment #8609201 - Attachment is obsolete: true
Attachment #8609201 - Flags: review?(jmaher)
Attachment #8609243 - Flags: review?(jmaher)
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 (
Attachment #8609243 - Flags: review?(jmaher) → review-
(Assignee)

Comment 21

2 years ago
Created attachment 8610328 [details] [diff] [review]
[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.
Attachment #8609243 - Attachment is obsolete: true
Attachment #8610328 - Flags: review?(jmaher)
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.
Attachment #8610328 - Flags: review?(jmaher) → review+
(Assignee)

Comment 23

2 years ago
Created attachment 8611124 [details] [diff] [review]
Add expire setting of permissions to SpecialPowers r=jmaher

- add a space ' ' before the ? in trinary operator
- carry r+
Attachment #8610328 - Attachment is obsolete: true
Attachment #8611124 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 24

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/020ac30e270a
Keywords: checkin-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
Flags: needinfo?(cchang)
(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

2 years ago
Backout:
https://hg.mozilla.org/mozilla-central/rev/8707d35414f4
(Assignee)

Comment 28

2 years ago
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
Flags: needinfo?(cchang)
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).
(Assignee)

Comment 30

2 years ago
Created attachment 8612709 [details] [diff] [review]
[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
Attachment #8611124 - Attachment is obsolete: true
Attachment #8612709 - Flags: review?(jmaher)
(Assignee)

Updated

2 years ago
Attachment #8612709 - Flags: review?(jmaher)
Attachment #8612709 - Flags: review-
Attachment #8612709 - Flags: feedback?(jmaher)
(Assignee)

Comment 31

2 years ago
Created attachment 8612785 [details] [diff] [review]
[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?
Attachment #8612709 - Attachment is obsolete: true
Attachment #8612709 - Flags: feedback?(jmaher)
Attachment #8612785 - Flags: feedback?(jmaher)
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?
Attachment #8612785 - Flags: feedback?(jmaher) → feedback+
(Assignee)

Comment 33

2 years ago
(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;
  }
}
(Assignee)

Comment 34

2 years ago
(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
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?
(Assignee)

Comment 36

2 years ago
(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.
(Assignee)

Comment 37

2 years ago
(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.
do you know the time difference on winxp?  is this something large enough to account for a timezone issue?  Maybe we are lacking precision?
(Assignee)

Comment 39

2 years ago
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?
(Assignee)

Comment 40

2 years ago
[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
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.
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.
oh ted!  great.
(Assignee)

Comment 44

2 years ago
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
(Assignee)

Comment 45

2 years ago
Created attachment 8615209 [details] [diff] [review]
[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
Attachment #8612785 - Attachment is obsolete: true
Attachment #8615209 - Flags: review?(jmaher)
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:)
Attachment #8615209 - Flags: review?(jmaher) → review+
(Assignee)

Updated

2 years ago
See Also: → bug 1171443
(Assignee)

Comment 47

2 years ago
(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
(Assignee)

Comment 48

2 years ago
Created attachment 8615890 [details] [diff] [review]
[plan B] Add expire setting of permission to SpecilaPowers and add  a systemTimestamp to nsIXULRuntime

Implementation of comment 47
(Assignee)

Comment 49

2 years ago
Created attachment 8616702 [details] [diff] [review]
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.
Attachment #8615209 - Attachment is obsolete: true
Attachment #8616702 - Flags: review?(jmaher)
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?
Attachment #8616702 - Flags: review?(jmaher) → review-
(Assignee)

Comment 51

2 years ago
Created attachment 8620821 [details] [diff] [review]
[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!
Attachment #8616702 - Attachment is obsolete: true
Attachment #8620821 - Flags: review?(jmaher)
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.
Attachment #8620821 - Flags: review?(jmaher) → review+
(Assignee)

Comment 53

2 years ago
Created attachment 8621917 [details] [diff] [review]
[v4.1] Add expire setting of permission to SpecilaPowers r=jmaher

move comment to TIME_SKEW and carry r+
Attachment #8615890 - Attachment is obsolete: true
Attachment #8620821 - Attachment is obsolete: true
Attachment #8621917 - Flags: review+
(Assignee)

Comment 54

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fed935e708dc
Keywords: checkin-needed

Comment 55

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc4bddc95462
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cc4bddc95462
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1175263
You need to log in before you can comment on or make changes to this bug.