Closed
Bug 1037235
Opened 10 years ago
Closed 10 years ago
toolkit/loader doesn't check module compatibility
Categories
(Add-on SDK Graveyard :: General, defect, P1)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: mossop, Assigned: evold)
References
Details
Attachments
(6 files)
44 bytes,
text/x-github-pull-request
|
mossop
:
review+
|
Details | Review |
44 bytes,
text/x-github-pull-request
|
Details | Review | |
44 bytes,
text/x-github-pull-request
|
mossop
:
review+
past
:
review+
Yoric
:
feedback+
|
Details | Review |
29.90 KB,
patch
|
mossop
:
review+
Yoric
:
feedback+
|
Details | Diff | Splinter Review |
41.23 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
52 bytes,
text/plain
|
jsantell
:
review+
|
Details |
The cuddlefish loader verifies that a module is compatible with the current application. toolkit/loader doesn't do this.
Reporter | ||
Updated•10 years ago
|
Priority: -- → P1
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → evold
Assignee | ||
Updated•10 years ago
|
Blocks: native-jetpack
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8460046 -
Flags: review?(dtownsend+bugmail)
Reporter | ||
Updated•10 years ago
|
Attachment #8460046 -
Flags: review?(dtownsend+bugmail) → review+
Assignee | ||
Comment 2•10 years ago
|
||
ttps://hg.mozilla.org/integration/fx-team/rev/74f015f58be9
Assignee | ||
Comment 3•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/74f015f58be9
I forgot the r=Mossop :(
Reporter | ||
Comment 4•10 years ago
|
||
This looks to be causing multiple test failures in the devtools xpcshell tests. One example:
12:10:15 WARNING - TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | Source file C:/slave/test/build/tests/xpcshell/tests/browser/devtools/shared/test/unit/test_bezierCanvas.js contains an error
12:10:15 INFO - Diagnostic: NS_ERROR_XPC_GS_RETURNED_FAILURE: Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]
12:10:15 INFO - JS frame :: resource://gre/modules/sdk/system/XulApp.js:13 :: anonymous :: line 14
12:10:15 INFO - JS frame :: Loader@resource://gre/modules/commonjs/toolkit/loader.js:47:4
12:10:15 INFO - JS frame :: resource://gre/modules/commonjs/toolkit/loader.js:12 :: anonymous :: line 0
12:10:15 INFO - JS frame :: @resource://gre/modules/commonjs/toolkit/loader.js:5:0
12:10:15 INFO - JS frame :: resource://gre/modules/devtools/Loader.jsm:33 :: anonymous :: line 13
12:10:15 INFO - JS frame :: @C:/slave/test/build/tests/xpcshell/tests/browser/devtools/shared/test/unit/test_bezierCanvas.js:11:17
12:10:15 INFO - JS frame :: C:\\slave\\test\\build\\tests\\xpcshell\\head.js:510 :: loadTailFile :: line 6
12:10:15 INFO - JS frame :: _load_files@C:\\slave\\test\\build\\tests\\xpcshell\\head.js:526:2
12:10:15 INFO - JS frame :: C:\\slave\\test\\build\\tests\\xpcshell\\head.js:393 :: _execute_test :: line 2
12:10:15 INFO - JS frame :: @-e:1:0
Flags: needinfo?(evold)
Assignee | ||
Comment 6•10 years ago
|
||
https://github.com/erikvold/gecko-dev/compare/mozilla:master...erikvold:1037235v2
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0c1d99ce53a9
Flags: needinfo?(evold)
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8469746 -
Flags: review?(dtownsend+bugmail)
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #8)
> Created attachment 8469746 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/42
https://tbpl.mozilla.org/?tree=Try&rev=6e6095a7be62
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8469746 [details] [review]
Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/42
Looks good to me.
Yoric, can you check the changes to the osfile tests to make sure they're sane?
I'd also like someone with better knowledge of the devtools tests to sign off on those, past maybe?
Attachment #8469746 -
Flags: review?(past)
Attachment #8469746 -
Flags: review?(dtownsend+bugmail)
Attachment #8469746 -
Flags: review?(dteller)
Attachment #8469746 -
Flags: review+
I don't understand why these patches touch OS.File at all, since OS.File doesn't use toolkit/loader. What am I missing?
Flags: needinfo?(evold)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] (away until August 20th - use "needinfo") from comment #11)
> I don't understand why these patches touch OS.File at all, since OS.File
> doesn't use toolkit/loader. What am I missing?
https://tbpl.mozilla.org/php/getParsedLog.php?id=44540605&tree=Fx-Team&full=1#error15
Due to https://github.com/erikvold/gecko-dev/blob/1037235v4/toolkit/components/osfile/tests/xpcshell/test_loader.js
Flags: needinfo?(evold)
Assignee | ||
Comment 13•10 years ago
|
||
This is a list of the errors https://tbpl.mozilla.org/php/getParsedLog.php?id=44540605&tree=Fx-Team&full=1
Comment 14•10 years ago
|
||
Comment on attachment 8469746 [details] [review]
Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/42
The devtools-related changes look good to me.
Attachment #8469746 -
Flags: review?(past) → review+
Comment on attachment 8469746 [details] [review]
Link to Github pull-request: https://github.com/mozilla/gecko-dev/pull/42
Reviewing only the osfile part.
This patch removes a test on the OS name. Can you fix this?
Also, I don't understand the interest of nsIXulAppInfo in the OS.File tests.
Attachment #8469746 -
Flags: review?(dteller) → feedback+
Assignee | ||
Comment 16•10 years ago
|
||
I've updated the osfile test changes.
Attachment #8476024 -
Flags: review?(dtownsend+bugmail)
Attachment #8476024 -
Flags: feedback?(dteller)
Reporter | ||
Updated•10 years ago
|
Attachment #8476024 -
Flags: review?(dtownsend+bugmail) → review+
Comment on attachment 8476024 [details] [diff] [review]
1037235v4.diff
Review of attachment 8476024 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me, thanks.
Attachment #8476024 -
Flags: feedback?(dteller) → feedback+
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
Hmm I guess I need to gain some insight on the B2G tests now..
Flags: needinfo?(evold)
Assignee | ||
Comment 20•10 years ago
|
||
Trying again https://tbpl.mozilla.org/?tree=Try&rev=5a25d760bbdc
Comment 21•10 years ago
|
||
I think that because of this exception:
Marionette ERROR exception: Error, resource://gre/modules/sdk/system/XulApp.js - EXPORTED_SYMBOLS is not an array.
On b2g, global scopes in JSMs are a bit weird...
you have to do:
this.EXPORTED_SYMBOLS = ["XulApp"];
as well as:
this.XulApp = ...;
Otherwise you wouldn't have had to modify randoms tests to fix AppInfo
if `incompatibility` function was moved to loader.js...
It's not really clear why it ends up in XulApp.js whereas it is a loader thing.
I could have understood if incompatibility wasn't specific to loader,
and let's say, being given by cuddlefish.js through Loader constructor argument,
but now it is explicitely imported from loader.js header.
Assignee | ||
Comment 22•10 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #21)
> I think that because of this exception:
> Marionette ERROR exception: Error,
> resource://gre/modules/sdk/system/XulApp.js - EXPORTED_SYMBOLS is not an
> array.
>
> On b2g, global scopes in JSMs are a bit weird...
> you have to do:
> this.EXPORTED_SYMBOLS = ["XulApp"];
> as well as:
> this.XulApp = ...;
Ah thank you Alex, that seems to work!
https://tbpl.mozilla.org/?tree=Try&rev=3517fe20fb6a
> Otherwise you wouldn't have had to modify randoms tests to fix AppInfo
> if `incompatibility` function was moved to loader.js...
> It's not really clear why it ends up in XulApp.js whereas it is a loader
> thing.
I'll be using this in the AOM to check that add-ons are compatible later, that's why this bug blocks bug 915376.
Assignee | ||
Comment 23•10 years ago
|
||
Comment 24•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 25•10 years ago
|
||
Commits pushed to master at https://github.com/mozilla/addon-sdk
https://github.com/mozilla/addon-sdk/commit/23bc56c8ba8ea07d1fa1975e12731b37fd9d7e5d
Bug 1037235 - toolkit/loader doesn't check module compatibility r=Mossop
https://github.com/mozilla/addon-sdk/commit/6febd6e84df2711e781beadd6ee9e0d58589296b
Merge pull request #1621 from erikvold/bug1037235
Bug 1037235 - toolkit/loader doesn't check module compatibility r=Mossop
Comment 26•10 years ago
|
||
WebIDE doesn't work anymore because of this bug. See bug 1067331.
Comment 27•10 years ago
|
||
Backed out as it breaks WebIDE: https://hg.mozilla.org/integration/fx-team/rev/4a953a406eaa
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 28•10 years ago
|
||
We got the following exception:
"Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIXULAppInfo.ID]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "JS frame :: resource://gre/modules/sdk/system/XulApp.js :: <TOP_LEVEL> :: line 19" data: no
Because XulApp.js was loaded within a child process, where nsIXulAppInfo xpcom doesn't work.
It ended up breaking b2g apps debugging because we are loading a DebuggerServer in the child process
and this DebuggerServer uses the SDK loader to load its modules.
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #22)
> I'll be using this in the AOM to check that add-ons are compatible later,
> that's why this bug blocks bug 915376.
I don't know what AOM means in term of code...
But again, I'm not sure we want to pull XulApp.js from loader.js, just for this "incompatiblity" method.
Assignee | ||
Comment 29•10 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #28)
> We got the following exception:
> "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE)
> [nsIXULAppInfo.ID]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"
> location: "JS frame :: resource://gre/modules/sdk/system/XulApp.js ::
> <TOP_LEVEL> :: line 19" data: no
>
> Because XulApp.js was loaded within a child process, where nsIXulAppInfo
> xpcom doesn't work.
> It ended up breaking b2g apps debugging because we are loading a
> DebuggerServer in the child process
> and this DebuggerServer uses the SDK loader to load its modules.
Where are these failing tests?
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #22)
> > I'll be using this in the AOM to check that add-ons are compatible later,
> > that's why this bug blocks bug 915376.
>
> I don't know what AOM means in term of code...
> But again, I'm not sure we want to pull XulApp.js from loader.js, just for
> this "incompatiblity" method.
AOM is Add-on Manager. It doesn't matter though, because the issue is that you guys are using the toolkit/loader which doesn't check incompatibility, and once we add that check it will use nsIXulAppInfo which doesn't work in the environment that you have setup. Putting the incompatibility function in the loader.js file won't make any difference.
So now we have to add nsIXulAppInfo to that environment.
Flags: needinfo?(paul)
Updated•10 years ago
|
Flags: needinfo?(paul) → needinfo?(poirot.alex)
Comment 30•10 years ago
|
||
Unfortunately, we don't have any test for this codepath.
That's why you got a green try and were able to land it.
To reproduce this issue you have to use webide and try to debug an app.
When you try to open a toolbox, it will silently fail.
With some more debugging code, you can see the exception I pasted.
You need to add try{...}catch(e){dump("exception in child process: "+e+"\n");} around the whole content of:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/child.js
to see the exception (I have no idea why this exception is trapped...)
Otherwise, I'm not sure we want to expose nsIXULAppInfo in child processes. I don't know why it is missing, but it might be on purpose.
Flags: needinfo?(poirot.alex)
Comment 31•10 years ago
|
||
I opened bug 1067424 to cover this with a unit test.
Assignee | ||
Comment 32•10 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #30)
> Unfortunately, we don't have any test for this codepath.
> That's why you got a green try and were able to land it.
>
> To reproduce this issue you have to use webide and try to debug an app.
How do I do this?
Flags: needinfo?(poirot.alex)
Comment 33•10 years ago
|
||
As it throws only in child processes, you would need a b2g device.
If you have one, open webide, connect to the phone and then select an app.
It should try to open a toolbox and will fail.
But if you never worked with a b2g phone, you might have a hard time debugging this.
The test I'm providing in bug 1067424 might help.
Flags: needinfo?(poirot.alex)
Comment 34•10 years ago
|
||
See attachment 8489438 [details] [diff] [review] from bug 1067424. It provides a patch that fails with your patch applied.
You can run this mochitest-plain on Firefox desktop, it will also fails there.
Assignee | ||
Comment 35•10 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #33)
> As it throws only in child processes, you would need a b2g device.
I've been trying to get a device for ages, still no luck.
> If you have one, open webide, connect to the phone and then select an app.
> It should try to open a toolbox and will fail.
> But if you never worked with a b2g phone, you might have a hard time
> debugging this.
> The test I'm providing in bug 1067424 might help.
I'm guessing this is going to take me a long time to fix, since I don't have a phone still and haven't been able to use one.
Can someone on the webide team handle this?
Flags: needinfo?(paul)
Comment 36•10 years ago
|
||
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #35)
> Can someone on the webide team handle this?
Yes.
Flags: needinfo?(paul)
Comment 37•10 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #30)
> Otherwise, I'm not sure we want to expose nsIXULAppInfo in child processes.
> I don't know why it is missing, but it might be on purpose.
there is a bug 1061873 for this (also a patch in bug 1067229), and consensus there seems to be "yes we want to"..
This also broke e10s devtools. You can test by opening the browser console with the "Enable E10S" box checked in about:preferences. In e10s it won't open.
Comment 39•10 years ago
|
||
(In reply to Tomislav Jovanovic [:zombie] from comment #37)
> (In reply to Alexandre Poirot [:ochameau] from comment #30)
> > Otherwise, I'm not sure we want to expose nsIXULAppInfo in child processes.
> > I don't know why it is missing, but it might be on purpose.
>
> there is a bug 1061873 for this (also a patch in bug 1067229), and consensus
> there seems to be "yes we want to"..
Oh great, thanks for mentioning these bugs!
It looks like we should be able to reland once bug 1067229 lands.
Comment 40•10 years ago
|
||
Commit pushed to master at https://github.com/mozilla/addon-sdk
https://github.com/mozilla/addon-sdk/commit/d830d8823c589920a108e61706f6f0edc48a278d
Backed out changeset (bug 1037235) for breaking WebIDE (bug 1067331)
Revert "Bug 1037235 - toolkit/loader doesn't check module compatibility r=Mossop"
This reverts commit 23bc56c8ba8ea07d1fa1975e12731b37fd9d7e5d.
Depends on: 1067229
Assignee | ||
Comment 41•10 years ago
|
||
I'm not sure who this should be assigned to now.
Assignee: evold → nobody
Flags: needinfo?(paul)
Assignee | ||
Comment 42•10 years ago
|
||
Alex said the WebIDE issue is resolved and has a test now, so I'm trying again:
https://tbpl.mozilla.org/?tree=Try&rev=3f2dc8003da7
Flags: needinfo?(paul)
Assignee | ||
Comment 43•10 years ago
|
||
(In reply to Erik Vold [:erikvold] (work week -> pto) from comment #42)
> Alex said the WebIDE issue is resolved and has a test now, so I'm trying
> again:
>
> https://tbpl.mozilla.org/?tree=Try&rev=3f2dc8003da7
Bug 1032017 was causing failures..
Depends on: 1032017
Assignee | ||
Comment 44•10 years ago
|
||
Trying again:
https://tbpl.mozilla.org/?tree=Try&rev=7ed2af4a2d94
Assignee | ||
Comment 45•10 years ago
|
||
(In reply to Erik Vold [:erikvold] (work week -> pto) from comment #44)
> Trying again:
>
> https://tbpl.mozilla.org/?tree=Try&rev=7ed2af4a2d94
Hmm I get the same failures, not sure if this test needs the appinfo hack or not, but I'm trying again now with this hack added to the failing test.
https://tbpl.mozilla.org/?tree=Try&rev=a53a614cb12e
Assignee | ||
Comment 47•10 years ago
|
||
Jaws, I just need a r? from you on the loop test change.
Attachment #8501523 -
Flags: review?(jaws)
Updated•10 years ago
|
Attachment #8501523 -
Attachment is patch: true
Comment 48•10 years ago
|
||
Comment on attachment 8501523 [details] [diff] [review]
a53a614cb12e
Review of attachment 8501523 [details] [diff] [review]:
-----------------------------------------------------------------
r=me for the loop xpcshell head.js change.
Attachment #8501523 -
Flags: review?(jaws) → review+
Comment 49•10 years ago
|
||
Not sure what changed between the time of the Try push in comment 45 and the time it was pushed to fx-team (no comment in the bug???), but it caused mass xpcshell failure. Backed out :(
https://hg.mozilla.org/integration/fx-team/rev/18f5b1df4794
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=851608&repo=fx-team
Please do comment in the bug when you push in the future, btw.
Assignee | ||
Comment 50•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #49)
> Please do comment in the bug when you push in the future, btw.
What comment were you looking for?
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 51•10 years ago
|
||
(In reply to Erik Vold [:erikvold] (work week -> pto) from comment #50)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #49)
> > Please do comment in the bug when you push in the future, btw.
>
> What comment were you looking for?
Ah right, I'm used to this being automated, sorry.
https://hg.mozilla.org/integration/fx-team/rev/4205a47f317c
If you want to somewhat automate the commenting, you can run mc-merge against your push. When you look at your push on TBPL or Treeherder, you click the mc-merge link, click the Submit button at the bottom of the page, give it your bugzilla credentials, and it will comment in the bug as you. Not really any quicker than just commenting manually if you're only pushing one patch for a single bug, but it's a bit more convenient when a bug has multiple patches or you're pushing multiple bugs at once, since it comments on them all for you.
The mc-merge link in TBPL shows itself when you hover the cursor over your push. It's on the right side of the page.
Treeherder hides the mc-merge link in a menu towards the left of the page. At the moment, it's under the triangle icon, but that might change as there's a bug to make those icons more self-explanatory.
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 53•10 years ago
|
||
Comment 54•10 years ago
|
||
It seems that adding module compatibility checks now breaks bunch of code from other teams that would take too much time from us to fix (given the lack of resources and knowledge of the code base). There for I suggest to update a patch so that compatibility checks are opt-in. Meaning that we should add additional option to a loader:
Loader({
checkCompatibility: 0
})
does not check compatibility
Loader({
checkCompatibility: 1
})
throws exception on incompatibility
SDK code could opt in by providing `checkCompatibility: 1` while devtools code will continue working as it used to as they won't opt-in. Or will fix issues before they opt-in.
Assignee | ||
Updated•10 years ago
|
Blocks: toolkit/loader
Assignee | ||
Comment 55•10 years ago
|
||
Attachment #8542260 -
Flags: review?(jsantell)
Updated•10 years ago
|
Attachment #8542260 -
Flags: review?(jsantell) → review+
Comment 56•10 years ago
|
||
Commits pushed to master at https://github.com/mozilla/addon-sdk
https://github.com/mozilla/addon-sdk/commit/66a13fa81904e6dbbd496c03b831b25a014ba0e5
Bug 1037235 - toolkit/loader does not check module compatibility
https://github.com/mozilla/addon-sdk/commit/cf34e6908133d67254adc46d9e66f387b4b09bc1
Bug 1037235 - removing a redundant test from test-cuddlefish.js, fixing test-loader.js tests, and enabling the test-unsupported-skip.js test on travis a=me
https://github.com/mozilla/addon-sdk/commit/12391f89c0fc82fbd4683c87df2aee8dfc58772d
Merge pull request #1779 from erikvold/1037235v5
Bug 1037235 - toolkit/loader now can opt-in for module compatibility r=jsantell
Assignee | ||
Updated•10 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•