Closed
Bug 816847
Opened 12 years ago
Closed 11 years ago
SpecialPowers.addPermission fails OOP due to importing WebApps.jsm
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(firefox19 fixed, firefox20 fixed, b2g18 fixed)
RESOLVED
FIXED
mozilla20
People
(Reporter: pauljt, Assigned: dchanm+bugzilla)
References
Details
Attachments
(1 file, 2 obsolete files)
7.42 KB,
patch
|
jgriffin
:
review+
|
Details | Diff | Splinter Review |
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);
Reporter | ||
Updated•12 years ago
|
Summary: SpecialPowers.addPermission fails OOP due to call to importing WebApps.jsm → SpecialPowers.addPermission fails OOP due to importing WebApps.jsm
Assignee | ||
Updated•12 years ago
|
Component: DOM: Device Interfaces → Mochitest
Product: Core → Testing
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
I am writing a test for this, but this should work Where do tests for specialpowers go?
Assignee: nobody → dchan+bugzilla
Comment 3•12 years ago
|
||
(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
Assignee | ||
Comment 4•12 years ago
|
||
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 5•12 years ago
|
||
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+
Reporter | ||
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
(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
Comment 8•12 years ago
|
||
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.
Assignee | ||
Comment 9•12 years ago
|
||
(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.
Assignee | ||
Comment 10•12 years ago
|
||
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.
Comment 11•12 years ago
|
||
(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.
Assignee | ||
Comment 12•11 years ago
|
||
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 13•11 years ago
|
||
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+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/23c55acae9e3
Flags: in-testsuite+
Keywords: checkin-needed
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/23c55acae9e3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Updated•11 years ago
|
Whiteboard: [automation-needed-in-aurora][automation-needed-in-b2g18]
Comment 16•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/8fdb36d0b7eb https://hg.mozilla.org/releases/mozilla-b2g18/rev/02c605cbc6fe
status-b2g18:
--- → fixed
status-firefox19:
--- → fixed
status-firefox20:
--- → fixed
Whiteboard: [automation-needed-in-aurora][automation-needed-in-b2g18]
Comment 17•8 years ago
|
||
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)
Assignee | ||
Comment 18•8 years ago
|
||
(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)
Comment 19•8 years ago
|
||
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)
Comment 20•8 years ago
|
||
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.
Assignee | ||
Comment 21•8 years ago
|
||
(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)
Comment 22•8 years ago
|
||
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.
Description
•