Closed Bug 816847 Opened 9 years ago Closed 9 years ago

SpecialPowers.addPermission fails OOP due to importing WebApps.jsm

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(firefox19 fixed, firefox20 fixed, b2g18 fixed)

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: pauljt, Assigned: dchanm+bugzilla)

References

Details

Attachments

(1 file, 2 obsolete files)

SpecialPowers.addPermissions fail OOP when passing an app object. I get this error:
ERROR TEST-UNEXPECTED-FAIL | /tests/webapi/test_all_permissions.html | an unexpected uncaught JS exception reported through window.onerror - NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService] at resource://gre/modules/XPCOMUtils.jsm:202

Based on help from dchan, I think this is because SpecialPowers tries to call:
Cu.import("resource://gre/modules/Webapps.jsm", tmp);

Which fails out of process. The code in question is here: http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#1246

There is a workaround in bug 811141 (https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=811141&attachment=686784)

I think instead of:
tmp = {};
Cu.import("resource://gre/modules/Webapps.jsm", tmp);
let app = tmp.DOMApplicationRegistry.getAppByManifestURL(arg.manifestURL);

there can be  a workaround by using the appsService:

const appsService = Cc["@mozilla.org/AppsService;1"].getService(Ci.nsIAppsService);
let app = appsService.getAppByManifestURL(aURI);
Summary: SpecialPowers.addPermission fails OOP due to call to importing WebApps.jsm → SpecialPowers.addPermission fails OOP due to importing WebApps.jsm
Component: DOM: Device Interfaces → Mochitest
Product: Core → Testing
Yes, Webapps.jsm is only usable from the parent process. The AppsService does the right thing in both the content and parent processes, so you can use it safely. Let me know if you need more functionnality exposed on this service.
I am writing a test for this, but this should work

Where do tests for specialpowers go?
Assignee: nobody → dchan+bugzilla
(In reply to David Chan [:dchan] from comment #2)
> Created attachment 687186 [details] [diff] [review]
> Change to use AppsService - untested
> 
> I am writing a test for this, but this should work
> 
> Where do tests for specialpowers go?

See e.g., http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/test_SpecialPowersExtension.html?force=1
There was a bit missing from the previous patch where it didn't get localId correctly. The cloned apps object from AppsService doesn't expose localId due to the interface set I believe.

I also added some tests to check the 4 branches of addPermission/removePermission and a check that setting permission on app isn't set on normal webpage w/o appId.
Attachment #687186 - Attachment is obsolete: true
Attachment #689001 - Flags: review?(jgriffin)
Comment on attachment 689001 [details] [diff] [review]
Change to us AppsService v2 + tests

Looks ok to me.  I pushed this to try to make sure nothing is getting broken accidentally: https://tbpl.mozilla.org/?tree=Try&rev=dfd529ad4110
Attachment #689001 - Flags: review?(jgriffin) → review+
Comment on attachment 689001 [details] [diff] [review]
Change to us AppsService v2 + tests

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

::: testing/mochitest/tests/test_SpecialPowersExtension.html
@@ +186,5 @@
> +  // test that addPermission and removePermission
> +  var allow = Ci.nsIPermissionManager.ALLOW_ACTION;
> +  var deny = Ci.nsIPermissionManager.DENY_ACTION;
> +  var unknown = Ci.nsIPermissionManager.UNKNOWN_ACTION;
> +  var perms = ['resource-lock', 'geolocation', 'camera', 'alarms']

I don't think 'resource-lock' exists any more. Will this break your test?

@@ +204,5 @@
> +  SpecialPowers.addPermission(perms[0], "allow", origin);
> +  SpecialPowers.addPermission(perms[1], "allow", {manifestURL: manifest});
> +  SpecialPowers.addPermission(perms[2], "allow", document);
> +  SpecialPowers.addPermission(perms[3], "allow", {url: origin,
> +                                                  appId: Ci.nsIScriptSecurityManager.NO_APP_ID});

Did you mean to quote the "allow" strings above? I think this should actually be |true|, unquoted.
(In reply to Paul Theriault [:pauljt] from comment #6)
> Comment on attachment 689001 [details] [diff] [review]
> Change to us AppsService v2 + tests
> 
> Review of attachment 689001 [details] [diff] [review]:
> -----------------------------------------------------------------
> > +  var perms = ['resource-lock', 'geolocation', 'camera', 'alarms']
> 
> I don't think 'resource-lock' exists any more. Will this break your test?

I don't think it will affect tests since you can make up any permission name. However I will change it to another permission.

> 
> @@ +204,5 @@
> > +  SpecialPowers.addPermission(perms[3], "allow", {url: origin,
> > +                                                  appId: Ci.nsIScriptSecurityManager.NO_APP_ID});
> 
> Did you mean to quote the "allow" strings above? I think this should
> actually be |true|, unquoted.

Good catch, |true| is a better value to use. addPermission does a truthiness test which meant "allow" resulted in the correct behavior, but I will change that
The try run failed on android with:

34 ERROR TEST-UNEXPECTED-FAIL | /tests/Harness_sanity/test_SpecialPowersExtension.html | Got an app
35 ERROR TEST-UNEXPECTED-FAIL | /tests/Harness_sanity/test_SpecialPowersExtension.html | uncaught exception - TypeError: app is null at http://mochi.test:8888/tests/Harness_sanity/test_SpecialPowersExtension.html:199

Once you have a new patch, I'll re-run it through try.
(In reply to Jonathan Griffin (:jgriffin) from comment #8)
> The try run failed on android with:
> 
> 34 ERROR TEST-UNEXPECTED-FAIL |
> /tests/Harness_sanity/test_SpecialPowersExtension.html | Got an app
> 35 ERROR TEST-UNEXPECTED-FAIL |
> /tests/Harness_sanity/test_SpecialPowersExtension.html | uncaught exception
> - TypeError: app is null at
> http://mochi.test:8888/tests/Harness_sanity/test_SpecialPowersExtension.html:
> 199
> 
> Once you have a new patch, I'll re-run it through try.

Hmm weird. The code attempts to get the app with localId 1 which should be installed for testing purposes. I'll look for a better way of getting the list of apps. I just got access to try so I can test it there as well.
Comment 9 is private: false
Try push
https://tbpl.mozilla.org/?tree=Try&rev=cbfba01202ee

Are the leaks a bug in my code?

After thinking about it more, getting localId 1 seems really hacky. I could change the code to get one of the example.com manifests which may be slightly less magic.
(In reply to David Chan [:dchan] from comment #10)
> Try push
> https://tbpl.mozilla.org/?tree=Try&rev=cbfba01202ee
> 
> Are the leaks a bug in my code?
> 
> After thinking about it more, getting localId 1 seems really hacky. I could
> change the code to get one of the example.com manifests which may be
> slightly less magic.

Yes, it's likely it's caused by this patch, since the leaks are so consistent.
Made a slight change to how the app manifest was retrieved in tests.

Green try run
https://tbpl.mozilla.org/?tree=Try&rev=f48bb2ce2d23
Attachment #689001 - Attachment is obsolete: true
Attachment #691873 - Flags: review?(jgriffin)
Comment on attachment 691873 [details] [diff] [review]
Change to us AppsService v3 + tests

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

Cool, thanks for the try run.
Attachment #691873 - Flags: review?(jgriffin) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/23c55acae9e3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [automation-needed-in-aurora][automation-needed-in-b2g18]
I noticed that this test doesn't run in e10s mode, :dchan, can you comment on this so we can determine if we can fix this test to work in e10s?
Flags: needinfo?(dchanm+bugzilla)
(In reply to Joel Maher (:jmaher) from comment #17)
> I noticed that this test doesn't run in e10s mode, :dchan, can you comment
> on this so we can determine if we can fix this test to work in e10s?

Is there a log from the test failures? I'm assuming it's related to certain code paths not working properly when called from a different process.
Flags: needinfo?(dchanm+bugzilla)
I only mentioned this as it was turned off in the manifest:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/Harness_sanity/mochitest.ini#37

I assume there was a reason it didn't work.  I ran it locally on my linux64 desktop and saw a failure, here is the snippet from the log:
0 INFO SimpleTest START
1 INFO TEST-START | testing/mochitest/tests/Harness_sanity/test_bug816847.html
2 INFO TEST-PASS | testing/mochitest/tests/Harness_sanity/test_bug816847.html | Got an app  
3 INFO TEST-UNEXPECTED-FAIL | testing/mochitest/tests/Harness_sanity/test_bug816847.html | Set permission by string - got 0, expected 1
    SimpleTest.is@SimpleTest/SimpleTest.js:268:5
    starttest@testing/mochitest/tests/Harness_sanity/test_bug816847.html:62:3
    onload@testing/mochitest/tests/Harness_sanity/test_bug816847.html:1:1
4 INFO TEST-UNEXPECTED-FAIL | testing/mochitest/tests/Harness_sanity/test_bug816847.html | Set permission by manifestURL - got 0, expected 1
    SimpleTest.is@SimpleTest/SimpleTest.js:268:5
    starttest@testing/mochitest/tests/Harness_sanity/test_bug816847.html:64:3
    onload@testing/mochitest/tests/Harness_sanity/test_bug816847.html:1:1
5 INFO TEST-UNEXPECTED-FAIL | testing/mochitest/tests/Harness_sanity/test_bug816847.html | Set permission by principal - got 0, expected 1
    SimpleTest.is@SimpleTest/SimpleTest.js:268:5
    starttest@testing/mochitest/tests/Harness_sanity/test_bug816847.html:66:3
    onload@testing/mochitest/tests/Harness_sanity/test_bug816847.html:1:1
6 INFO TEST-UNEXPECTED-FAIL | testing/mochitest/tests/Harness_sanity/test_bug816847.html | Set permission by other - got 0, expected 1
    SimpleTest.is@SimpleTest/SimpleTest.js:268:5
    starttest@testing/mochitest/tests/Harness_sanity/test_bug816847.html:68:3
    onload@testing/mochitest/tests/Harness_sanity/test_bug816847.html:1:1
7 INFO TEST-PASS | testing/mochitest/tests/Harness_sanity/test_bug816847.html | Check that app permission doesn't leak to normal page 
8 INFO TEST-PASS | testing/mochitest/tests/Harness_sanity/test_bug816847.html | Removed permission by string 
9 INFO TEST-PASS | testing/mochitest/tests/Harness_sanity/test_bug816847.html | Removed permission by manifestURL 
10 INFO TEST-PASS | testing/mochitest/tests/Harness_sanity/test_bug816847.html | Removed permission by principal 
11 INFO TEST-PASS | testing/mochitest/tests/Harness_sanity/test_bug816847.html | Removed permission by other 
MEMORY STAT vsizeMaxContiguous not supported in this build configuration.
MEMORY STAT | vsize 537MB | residentFast 77MB | heapAllocated 22MB
12 INFO TEST-OK | testing/mochitest/tests/Harness_sanity/test_bug816847.html | took 1364ms


possibly the toggling of permissions in e10s is not possible?  running with --disable-e10s locally works just fine.
Flags: needinfo?(dchanm+bugzilla)
just a ping, we want to figure out if this should be running in e10s for e10s signoff- if it doesn't need to be that is great- lets just understand briefly why not.
(In reply to Joel Maher (:jmaher) from comment #20)
> just a ping, we want to figure out if this should be running in e10s for
> e10s signoff- if it doesn't need to be that is great- lets just understand
> briefly why not.

I don't believe this is a blocker for e10s, but I haven't kept up with the project. The tests check whether "webapps" are granted permissions correctly from a manifest file. The permissions code doesn't fail-open AFAIK, so webapps may end up running with reduced permissions under e10s.

Given the shift away from FxOS and the marketplace, these test can probably be deprecated.
Flags: needinfo?(dchanm+bugzilla)
I would be game for removing tests that are not needed- if I get the green light, I can do that.  Thanks for the response and we won't worry about this specific test for e10s.
You need to log in before you can comment on or make changes to this bug.