627073 - remove "AMD" define() scanner from securable-module.js?

Status: RESOLVED INCOMPLETE

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

Description

[Brian Warner [:warner :bwarner]]

In bug 596932, we added support for the async define() construct, which is
used by CommonJS modules that can be loaded in a web-page context (and thus
are hard-pressed to use the blocking require() function when expressing their
dependencies). define() tells the module loader what dependencies are
necessary, and provides a callback to actually create the new module after
those dependencies are loaded and ready to be used (which, in the web-page
world, may require a slow HTTP fetch from some server).

define() can be called in two basic modes. In the first, the deps are stated
explicitly, with a list of names. In the second, they are implicit: the
define() function is expected to stringify the callback to obtain its source
code, then grep through that code for "require()" statements that indicate
what deps are needed. The idea is that this makes module definitions easier
to write: the require() statements are necessary anyways (to obtain local
references to the dependent modules), and stating them a second time in the
explicit list-of-names is just repeating yourself, both annoying and an
opportunity for bugs.

The current define() code, when used in AMD mode, is using a regexp-based
scanner to compute the dependency list, pre-loading all those modules
synchronously, then invoking the callback. When the callback runs require(),
the fast "module was already loaded by somebody else" path is used, which
just returns the previously-cached module object.

The proposal under consideration is to remove the runtime (JS) scanner from
our define() implementation, mainly to reduce complexity. The goal of bug
596932 was to be able to take advantage of define()-using CommonJS modules,
so if a significant number of those modules use the "AMD" (implicit) mode,
we'd need to continue to support that. So this proposal is not about changing
our ability to support AMD-mode, but rather about the implementation.

Jetpack is loading all modules synchronously: we tolerate the async-enabling
define() construct, but in fact the require() calls inside the callback are
blocking the jetpack-process until the necessary code has been pulled in from
the browser-process side. So we don't have any particular need to learn the
dependencies ahead of time: we can just wait until the necessary-anyways
require() call is hit, find and load the given module, then return to the
next line of the module-defining callback function.

There are subtle reasons why this might not be sufficient: bug 596932 has
some discussion, but it's worth examining more closely. If it turns out that
we really do want to deduce the dependency list ahead of time (so that the
callback's require() functions return quickly), we might be able to rely upon
our out-of-band manifest.py scanner. This scanner looks through whole files
to populate the table of which-module-imports-which (used to enforce runtime
restrictions that match what add-on reviewers have looked at). We could
leverage that manifest data to pre-compute the dependency list without
performing decompilation and regexp-scanning at runtime.

Comment 1

[James Burke]

Just to add some comments that I started to put in bug 596932, but I'll put them here instead.

Brian Warner described why the scanner is there in this comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=596932#c40

The AMD API proposal says that the dependencies should be evaluated before calling the factory function, particularly for the define(['dependency'], function (dependency){}) case, otherwise the dependency's exports injection into that function will not work.

The runtime scanning is basically a way to build up that dependency array without the user having to explicitly code it. The load order stipulation is just that the dependency executed some time in the past before the current define factory function is called.

The scanner can be removed, given the sync nature of the Jetpack loader. It could mean that someone could author a module that runs in Jetpack that looks like so:

define(function(require){
  if (someCondition) {
    var a = require("a");
  } else {
    var a = require("a1");
  }
});

This could behave differently if this module is used on the web, maybe even error.

However, I think the solution to that is making it more explicit in the AMD proposal that any require("") calls basically have to happen at the top of the function and they cannot participate in logic branches. I think this is a fine thing to make explicit in the AMD proposal as it matches how "module" in Simple Modules would work.

Or, as Brian mentioned, the manifest.py scanning may be able to be leveraged, but I think the point of define() support is to allow sharing modules in web content, and I expect issues with badly coded modules to be found via that code reuse.

If it helps, I can do a patch that removes the runtime scanner, and have it done today, just let me know.

Comment 2

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

Brian, James: can you provide some input into how important this is to fix for the SDK 1.0 release?

Comment 3

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

Ping!  Any further thoughts on this?

Comment 4

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

Resolved incomplete due to lack of response; please reopen if you still care about this!