Closed Bug 1037235 Opened 10 years ago Closed 9 years ago

toolkit/loader doesn't check module compatibility

Categories

(Add-on SDK Graveyard :: General, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: mossop, Assigned: evold)

References

Details

Attachments

(6 files)

The cuddlefish loader verifies that a module is compatible with the current application. toolkit/loader doesn't do this.
Priority: -- → P1
Blocks: jpm
Assignee: nobody → evold
Depends on: 1040432
Attachment #8460046 - Flags: review?(dtownsend+bugmail) → review+
ttps://hg.mozilla.org/integration/fx-team/rev/74f015f58be9
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)
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)
(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)
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+
Attached patch 1037235v4.diffSplinter Review
I've updated the osfile test changes.
Attachment #8476024 - Flags: review?(dtownsend+bugmail)
Attachment #8476024 - Flags: feedback?(dteller)
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+
Hmm I guess I need to gain some insight on the B2G tests now..
Flags: needinfo?(evold)
Depends on: 905324
Flags: needinfo?(evold)
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/a6d02cff43d3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1067120
Depends on: 1067132
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
Depends on: 1067237
WebIDE doesn't work anymore because of this bug. See bug 1067331.
Backed out as it breaks WebIDE: https://hg.mozilla.org/integration/fx-team/rev/4a953a406eaa
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(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)
Flags: needinfo?(paul) → needinfo?(poirot.alex)
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)
I opened bug 1067424 to cover this with a unit test.
Depends on: 1067331
(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)
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)
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.
(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)
(In reply to Erik Vold [:erikvold] [:ztatic] from comment #35)
> Can someone on the webide team handle this?

Yes.
Flags: needinfo?(paul)
Depends on: 1067544
(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.
(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.
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.
I'm not sure who this should be assigned to now.
Assignee: evold → nobody
Flags: needinfo?(paul)
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)
(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
(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
Looks like this is mine again.
Assignee: nobody → evold
Attached patch a53a614cb12eSplinter Review
Jaws, I just need a r? from you on the loop test change.
Attachment #8501523 - Flags: review?(jaws)
Attachment #8501523 - Attachment is patch: true
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+
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.
(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)
(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)
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.
Attachment #8542260 - Flags: review?(jsantell) → review+
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
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
No longer depends on: 905324, 1067331
Depends on: 1122978
Blocks: 1143025
You need to log in before you can comment on or make changes to this bug.