627607 - switch to build-time module resolution (from run-time lookup)

Status: RESOLVED FIXED

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

Description

[Brian Warner [:warner :bwarner]]

Bug 591338 and bug 611495 both involve improvements to the way we do module-resolution in jetpack (the process that starts with require("foo") and ends with deciding which exact "foo.js" gets loaded). At present, we mainly do this resolution at runtime, in javascript, in securable-module.js (and others). But we also construct a manifest at buildtime (i.e. during "cfx xpi") and use it to restrict what the runtime loader is allowed to do.

I think it would be more reliable to consolidate this functionality into the SDK, and do only build-time resolution. The manifest would include a table that says "when module 4 does require('foo'), it should get module 5, which is loaded from FILE and has a hash of ABC123". Each require() function will then just do a simple table lookup, relying upon whatever $JSPATH-searching logic was done at build-time to generate that table.

One improvement of this change would be smaller XPIs: instead of including the full contents of any package that was around at build time, we'd only copy in the lib/ modules that are actually referenced. (we would need to copy in the full data/ directory of those packages, however, since the require("self").data methods are not constrained like require() itself).

One potential complication is with the test suite: previously we've allowed module-not-in-manifest to signal a warning, rather than an error, because we weren't sure exactly when test files ought to be included. We'd need to look more carefully at this, but I think it would be ok to skip the whole test/ subdirectory when creating an XPI.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Would modules that require("chrome") (or some other 'superuser' bit) still be allowed to dynamically require anything they please?

Comment 2

[Brian Warner [:warner :bwarner]]

(In reply to comment #1)

> Would modules that require("chrome") (or some other 'superuser' bit)
> still be allowed to dynamically require anything they please?

Good question. We certainly want to allow that. The trick is in the
bundling: if we don't know what they're going to ask for until run-time,
how can we arrange to include the files in the XPI at build time?

Atul and I were kicking around an idea on friday: we define a magic
module named "everything". If, at build-time, we notice anything doing
require("everything"), then we walk through every package on $JSPATH,
figure out how you might want to import them (maybe both short
require("panel") names and also the fully-qualified
require("addon-kit/panel")), and copy all those modules into the XPI.
Then a module could do something like:

  var require = require("everything").require;
  const dynamic = "panel"; var panel = require(dynamic);

If you don't use require("everything"), you get a small XPI but then
chrome-using modules' dynamically-constructed require() statements will
be limited to the XPI contents.

The use-case we've imagined for require("everything") would be a sort of
playground system: an addon that behaves like the old Jetpack Prototype,
with an in-browser text editor, on which you can type code one line at a
time and run it right away without the SDK and without a "cfx run" step.

Can you think of other use-cases for a dynamic require() statement? What
would you use it for?

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #2)
> Can you think of other use-cases for a dynamic require() statement? What
> would you use it for?

For me personally, I built an addon using the SDK, then did "cfx xpi" to create the XPI file. I then install that XPI into a Firefox profile and share it with the world.

Because of where my main SDK dev environment is located (Linux virtual machine on my Windows host), I usually try hacking on to the installed extension's folder/(packed xpi file) in the profile to add new features to the extension.

If those new features aren't covered by the modules already require()d by the extension back when "cfx xpi" was run, I have to require() the needed modules at run-time (or at least, when the browser restarts to pick up the changes). When I do this currently, I see the warning in the Error Console about how I shouldn't be doing this because in the future it won't work. But at the moment, this is the fastest way for me to play around with development. After I get a new feature (mostly) working in the installed extension, I then boot up the virtual machine and copy the new code over to the SDK and re-run "cfx xpi" to get a properly generated XPI file.

Maybe not a common use-case, but one that's valid for me.

Comment 4

[James Burke]

This capability sounds similar to how the RequireJS optimizer works:
http://requirejs.org/docs/optimization.html

It will bundle all the modules needed for for a project into one JS file. It loads each file, runs it through require.js, (but only a simplified, AST-parsed form that just has the dependencies) so it traces the dependency tree that way. The build profile has a way to specify additional modules that should be included that cannot be statically determined (so for example for dynamic require cases).

The optimizer runs as pure JS, with adapters to run inside Node or Rhino. It is probably possible to provide a Spidermonkey adapter too.

Since all the modules are bundled into one file (wrapped in AMD wrappers), that is probably different than what is desired for this bug, where the jetpack loader uses a separate context for each module, but it may also have a benefit of just having one file to load/checksum. And with that, a simplified loader could be used, which does not do other file loads. That said, it may be undesirable to have a different execution context and loader code for the built/vs non-built case for testing/consistency purposes.

It is probably not a direct fit for this use case, but just mentioning some tools or code that may be useful to help solve the issue.

Comment 5

[Brian Warner [:warner :bwarner]]

incidentally, https://github.com/warner/addon-sdk/tree/simpler-linker3 is where I'm currently working on this. As of this morning, I've got all the unit tests passing again except for the recent async/define ones: I'm still looking over that code to see what needs updating.

Comment 6

[Brian Warner [:warner :bwarner]]

Attachment: branch to review

Ok, this branch is finally passing all tests. I plan to clean it up a bit before asking for review for real, but Irakli it'd be great if you could start taking a look at it and give me some feedback.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Brian do you mind creating a pull request for that branch that way it will be easier to see the changes and put comments. Also your further cleanup changes will be picked up by it as long as you don't rebase.

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

So one thing that I noticed is:

addon-sdk)➜  repl  (master) cfx run
Warning: unable to satisfy require(./events) from ModuleInfo [moz-js lib net] (/Users/gozala/Projects/addon-sdk/packages/moz-js/lib/net.js, None)
Using binary at '/Applications/Firefox.app/Contents/MacOS/firefox-bin'.
Using profile at '/var/folders/-y/-yjxO9IXGGSRN-hSaip1Qk+++TM/-Tmp-/tmpdK8eGW.mozrunner'.
TypeError: this._pathMap[path] is undefined (resource://jep4repl-api-utils-lib/securable-module.js:554)
stack:
getFile("resource://jep4repl-api-utils-lib/unload.js")@resource://jep4repl-api-utils-lib/securable-module.js:554
loadFromModuleData([object Proxy])@resource://jep4repl-api-utils-lib/securable-module.js:293
loadMaybeMagicModule("unload",[object Proxy])@resource://jep4repl-api-utils-lib/securable-module.js:280
syncRequire("unload")@resource://jep4repl-api-utils-lib/securable-module.js:229
asyncRequire("unload")@resource://jep4repl-api-utils-lib/securable-module.js:345
@:0
@resource://jep4repl-api-utils-lib/securable-module.js -> resource://jep4repl-api-utils-lib/memory.js:137

FAIL
Total time: 1.849899 seconds
Program terminated unsuccessfully. 

What is interesting there is that package I'm trying to run does not depends on "moz-js" package which I have in my packages folder, it's just happens so that moz-js and my `net` package both have `net` modules.

Comment 9

[Brian Warner [:warner :bwarner]]

(I want to rewrite the patch before adding the pull-request, and rebase it as well, but I'll try to do that tomorrow, and then stop rebasing it)

Weird. If you build the XPI and look in harness-options.json at the manifest, can you see how moz-js/net.js is being included? Do you think that the dependency-search code is finding the wrong module?

Maybe a module is using require("net") intending to grab the "net" package's main.js, but the linker is instead satisfying the dependency with moz-js/net.js (from the wrong package altogether)? I don't think we have any support for the "main" property in package.json yet.

Comment 10

[Brian Warner [:warner :bwarner]]

Comment on attachment 525882 [details]
branch to review

switched to a different branch

Comment 11

[Brian Warner [:warner :bwarner]]

Attachment: pointer to pull request 148

here's the new rebased/rewritten branch. Tests finally pass, and some debug noise has been cleaned up. Ready for review.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Hmm, I think we're targeting this to 1.0b5.  That's what the 1.0b5 plan claims, and it's a big enough change that we should get beta coverage for it.

Comment 13

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Brian, I managed to get my head around it and peer review we talked about is no longer necessary. Overall I'm happy with a change but I have a few nits and open questions (see pull request). I'd like to hear from you on before accepting this, I'll be online.

Comment 14

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 527319 [details]
pointer to pull request 148

After discussing few questions I had on IRC, I'm happy with a change.
Thanks brian!!

Comment 15

[Brian Warner [:warner :bwarner]]

Landed, with a=myk, in https://github.com/mozilla/addon-sdk/commit/bf1d987e99c7fa8c056cf511cae0d7640af3f4c7
 . Yay!

Comment 16

[Brian Warner [:warner :bwarner]]

oops, I missed a unixism (assuming os.path.sep=="/"), and tests are failing on windows.

Comment 17

[Nickolay_Ponomarev]

> One improvement of this change would be smaller XPIs
Was this actually implemented? (I'm getting a large XPI when building and not sure if it's just me.) If not, do you plan to work on it and is there a separate bug on that?

Comment 18

[Brian Warner [:warner :bwarner]]

(In reply to comment #17)
> > One improvement of this change would be smaller XPIs
> Was this actually implemented? (I'm getting a large XPI when building and not
> sure if it's just me.)

Alas, I had to defer that feature to get the patch small enough to review in time for 1.0b5 (I'd actually forgotten that I made that decision a month ago). I've opened Bug 653256 to track that one, and I'll try to get it in place for 1.0 .

Comment 19

[Brian Warner [:warner :bwarner]]

buildbot is green once more, so I think I nailed the slash issue. Reclosing.

Comment 20

[Nickolay_Ponomarev]

> Alas, I had to defer that feature
Thanks for the clarification!