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

Status: RESOLVED DUPLICATE of bug 1309350

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

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Dave Townsend [:mossop]]

Attachment: pull request

Comment 2

[[github robot]]

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

Comment 3

[[github robot]]

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.

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

(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.

Comment 5

[Dave Townsend [:mossop]]

(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?

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

(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.

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

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.

Comment 9

[Dave Townsend [:mossop]]

(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?

Comment 10

[Dave Townsend [:mossop]]

Attachment: 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.

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

(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?

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

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 13

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8498334 [details] [review]
pull request

just some test changes, which is pretty much bug 1075454

Comment 14

[Dave Townsend [:mossop]]

(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.

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

(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).

Comment 16

[Dave Townsend [:mossop]]

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.