Closed Bug 1036625 Opened 5 years ago Closed 3 years ago

Loader shouldn't be attempting to load files just to resolve modules

Categories

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

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1309350

People

(Reporter: mossop, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
jsantell
: review-
Details | Review
Module resolution currently loads files to see if they exist. We should instead do the more complicated but cheaper method of resolving to a file/jar url and verifying the existence though the file or zip APIs.
Priority: -- → P2
Attached file pull request (obsolete) —
Assignee: nobody → dtownsend+bugmail
Status: NEW → ASSIGNED
Attachment #8494245 - Flags: review?(evold)
Attachment #8494245 - Flags: review?(evold) → review+
Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/337cd273188eea52e07aa355d6b0490d077cb1f0
Bug 1036625: Loader shouldn't be attempting to load files just to resolve modules.

https://github.com/mozilla/addon-sdk/commit/b8c654d1090c002a72cfdf2bc293696eef4621b7
Merge pull request #1643 from Mossop/bug1036625

Bug 1036625: Loader shouldn't be attempting to load files just to resolve modules. r=erikvold
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/ce0d80ab98834b71487568fd4c7c7da546e331f9
Revert "Bug 1036625: Loader shouldn't be attempting to load files just to resolve modules."

This reverts commit 337cd273188eea52e07aa355d6b0490d077cb1f0.
(In reply to [github robot] from comment #3)
> Commit pushed to master at https://github.com/mozilla/addon-sdk
> 
> https://github.com/mozilla/addon-sdk/commit/
> ce0d80ab98834b71487568fd4c7c7da546e331f9
> Revert "Bug 1036625: Loader shouldn't be attempting to load files just to
> resolve modules."
> 
> This reverts commit 337cd273188eea52e07aa355d6b0490d077cb1f0.

This caused a regression in using npm modules with jpm.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Erik Vold [:erikvold] (work week -> pto) from comment #4)
> (In reply to [github robot] from comment #3)
> > Commit pushed to master at https://github.com/mozilla/addon-sdk
> > 
> > https://github.com/mozilla/addon-sdk/commit/
> > ce0d80ab98834b71487568fd4c7c7da546e331f9
> > Revert "Bug 1036625: Loader shouldn't be attempting to load files just to
> > resolve modules."
> > 
> > This reverts commit 337cd273188eea52e07aa355d6b0490d077cb1f0.
> 
> This caused a regression in using npm modules with jpm.

What was the regression?
Flags: needinfo?(evold)
(In reply to Dave Townsend [:mossop] from comment #5)
> (In reply to Erik Vold [:erikvold] (work week -> pto) from comment #4)
> > (In reply to [github robot] from comment #3)
> > > Commit pushed to master at https://github.com/mozilla/addon-sdk
> > > 
> > > https://github.com/mozilla/addon-sdk/commit/
> > > ce0d80ab98834b71487568fd4c7c7da546e331f9
> > > Revert "Bug 1036625: Loader shouldn't be attempting to load files just to
> > > resolve modules."
> > > 
> > > This reverts commit 337cd273188eea52e07aa355d6b0490d077cb1f0.
> > 
> > This caused a regression in using npm modules with jpm.
> 
> What was the regression?

including npm modules with jpm doesn't work any longer, you can try with https://github.com/erikvold/test-addons/tree/pathfinder

Jordan and I looked at this for a couple of hours today and it looks like we have a test for this, so we're not sure why that was not failing at the moment.  We're going to investigate tomorrow.
Flags: needinfo?(evold)
This passes local tests with cfx because the addon isn't fully bootstrapped, and are just links to resource files, aka, unpacked. In real scenarios where the addon is packed (by default), this patch fails when a resource URI is checked in `fileExists` in this patch, and rerun as a jar:// uri and will always fail for any complex require (requiring a directory, or a dependency).

We should not land any changes like this until we have testing of a native addon loader in a real scenario, via bug 963226.
After talking with Jordan it sounds like the main issue with our test is two fold.  First the test is only run in "unpacked" mode and second it's run in an environment that isn't kept in sync with jpm's.

So I'm making this bug depend on bug 963226, and bug 1075454.
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #7)
> This passes local tests with cfx because the addon isn't fully bootstrapped,
> and are just links to resource files, aka, unpacked. In real scenarios where
> the addon is packed (by default), this patch fails when a resource URI is
> checked in `fileExists` in this patch, and rerun as a jar:// uri and will
> always fail for any complex require (requiring a directory, or a dependency).

So we have testaddons which test packed addons and were passing with this change, is this a difference between the cuddlefish loader and the native loader? Would a useful test for now be to have a testaddon create a native loader and attempt to load some things with it?
Attached file pull request
There was a tiny typo in the previous patch breaking checking inside JAR files. This corrects it and adds a test that uses the native loader to do module resolution. Without the typo fix the test fails. The test passes on current master and with the latest patch.

I think this test is good enough to allow us to land this now but it's your call. Unfortunately I can't get mochitest-jetpack tests running on tbpl until this issue is fixed though.
Attachment #8494245 - Attachment is obsolete: true
Attachment #8498334 - Flags: review?(jsantell)
(In reply to Dave Townsend [:mossop] from comment #10)
> I think this test is good enough to allow us to land this now but it's your
> call. Unfortunately I can't get mochitest-jetpack tests running on tbpl
> until this issue is fixed though.

Why is that?
Flags: needinfo?(dtownsend+bugmail)
Looks good, but the test case makes some tests that may work but aren't necessarily a rule (like requiring a file in node_modules/file.js via require("file")), and some things that were failing in the first patch but aren't tested here. The native loader unit test [1] tests all of these cases with a helper function (used in several tests here), and uses the native-addon-test [2] addon to scaffold these cases.

Can we test that native-addon-test (with the addon bootstrapping code you have in `test/addons/native-loader/main.js`, can be a dependency like `require("./native-addon-test")` and test all the exports there, and change the location of native-addon-test so the unit test in test/test-native-loader can also reach it

[1] https://github.com/mozilla/addon-sdk/blob/d0430978259a7b9bbe664ac4ef552d439b6736a4/test/test-native-loader.js#L183-L229
[2] https://github.com/mozilla/addon-sdk/tree/master/test/fixtures/native-addon-test
Comment on attachment 8498334 [details] [review]
pull request

just some test changes, which is pretty much bug 1075454
Attachment #8498334 - Flags: review?(jsantell) → review-
(In reply to Erik Vold [:erikvold] (work week -> pto) from comment #11)
> (In reply to Dave Townsend [:mossop] from comment #10)
> > I think this test is good enough to allow us to land this now but it's your
> > call. Unfortunately I can't get mochitest-jetpack tests running on tbpl
> > until this issue is fixed though.
> 
> Why is that?

Because the failed attempts to load files from various resource:// URIs logs an error and it does it so much that the debug logs overflow what tbpl can handle.
Flags: needinfo?(dtownsend+bugmail)
(In reply to Dave Townsend [:mossop] from comment #14)
> (In reply to Erik Vold [:erikvold] (work week -> pto) from comment #11)
> > (In reply to Dave Townsend [:mossop] from comment #10)
> > > I think this test is good enough to allow us to land this now but it's your
> > > call. Unfortunately I can't get mochitest-jetpack tests running on tbpl
> > > until this issue is fixed though.
> > 
> > Why is that?
> 
> Because the failed attempts to load files from various resource:// URIs logs
> an error and it does it so much that the debug logs overflow what tbpl can
> handle.

I see, well I think we'll need more tests (for instance a node modules that depends on another node_module), I think they'll be easier to land separately, and they will be cleaner if we do what I describe in bug 963226 (also they will be portable so that we can use them in the jpm test suite).
I updated my pull request with what I think Jordan was describing. I'm not a fan of sharing test code between the two different test types like that but it does work. However seems like there is some disagreement over doing this so for now I may drop this and instead try to unblock the mochitest work with bug 1077078.
Assignee: dtownsend+bugmail → nobody
Blocks: 1135219
Status: REOPENED → RESOLVED
Closed: 5 years ago3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1309350
You need to log in before you can comment on or make changes to this bug.