654095 - require misbehaves if unrelated packages contain modules with the same id

Status: RESOLVED FIXED

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

Comment 1

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

Most likely this is

Comment 2

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

Most likely this is related to the linker change:
https://bugzilla.mozilla.org/show_bug.cgi?id=627607

Comment 3

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

I can reproduce the issue by trying to `cfx run` following package https://github.com/Gozala/jetpack-test4jquery

It works fine if `packages` does not contains any custom packages, but if it does contains some other package (like https://github.com/Gozala/vats in my case) which contains a module with a conflicting name (In my case `events`) something in the linker fails and I see following error:

Traceback (most recent call last):
  File "resource://jetpack-test4jquery-api-utils-lib/content/loader.js", line 42, in null
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 385, in asyncRequire
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 236, in syncRequire
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 322, in loadMaybeMagicModule
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 333, in loadFromModuleData
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 593, in getFile
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 642, in loadFile
[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIIOService.newChannel]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://jetpack-test4jquery-api-utils-lib/securable-module.js :: loadFile :: line 642"  data: no]

Comment 4

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

I tried to investigate more into this issue and I found out that [manifest](https://gist.github.com/951645) passed to the `Loader` is invalid as it contains modules form the unrelated packages:

[resource://jetpack-test4jquery-vats-lib/events.js](https://gist.github.com/951645#L110)
[resource://jetpack-test4jquery-url-lib/url.js](https://gist.github.com/951645#L572)

In addition to that "content/loader" module [requires those two modules](https://gist.github.com/951645#L545) instead of expected "api-utils/url" and "api-utils/events" which are not even contained by a manifest.

I guess there is a bug in https://github.com/warner/addon-sdk/blob/master/python-lib/cuddlefish/manifest.py causing this.

Comment 5

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

Myk I think we should hold on b5 before this is fixed, as working packages may get broken by an unrelated packages around.

Comment 6

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

With Alex's help we found out the cause of this misbehavior: https://github.com/mozilla/addon-sdk/pull/148#r24878

In addition it looks like manifest builder does not takes package dependencies into account, which needs fixing as well (maybe as a separate bug).

Comment 7

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

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/154

Pointer to Github pull-request

Comment 8

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

Comment on attachment 529486 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/154

Attaching pull request which modifies manifest builder to make sure that module is searched in the same package before defaulting to the whole library.

Still this is not enough:

Traceback (most recent call last):
  File "resource://jetpack-test4jquery-addon-kit-lib/page-mod.js", line 43, in null
    const { EventEmitter } = require('events');
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 388, in asyncRequire
    return syncRequire(deps);
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 238, in syncRequire
    return loadMaybeMagicModule(module, reqs[module]);
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 325, in loadMaybeMagicModule
    return loadFromModuleData(moduleData); // adds to cache
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 336, in loadFromModuleData
    let moduleContents = self.fs.getFile(path);
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 596, in getFile
    return loadFile(path);
  File "resource://jetpack-test4jquery-api-utils-lib/securable-module.js", line 645, in loadFile
    var channel = ios.newChannel(path, null, null);
[Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIIOService.newChannel]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: resource://jetpack-test4jquery-api-utils-lib/securable-module.js :: loadFile :: line 645"  data: no]

Since when module from "addon-kit" makes require("events") it still gets "events" that is not from "api-utils" but from unrelated package. So we still need to narrow down search to a dependencies only.

Also this will only minimize the surface of the bug, as package still may depend on more then one package containing module with a same (generic) name. That's also why node went with explicit require("package/module/path") approach.

Comment 9

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

BTW I don't mind if someone more familiar with this part of code will take over this bug.

Comment 10

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

(In reply to comment #8)
> Attaching pull request which modifies manifest builder to make sure that module
> is searched in the same package before defaulting to the whole library.

If I remember correctly, the algorithm for module resolution is as follows (with "foo" being a package whose "bar" module requires the "baz" module):

1. If bar requires baz via an explicit package reference, look for baz in the specified package, i.e. require("foo/baz") looks for baz in foo.

2. If bar requires baz via a relative path, look for baz in the directory relative to the directory in which bar is located, i.e. require("./baz") looks for baz in the same directory as bar.

3. Otherwise, i.e. require("baz"), look for baz in the following places:

   a. the directory in which bar is located;
   b. the packages in the package path, as specified by the --package-path
      command-line argument, in the order in which the packages appear in that 
      path, i.e. require("baz") with --package-path=qux:qix looks for baz in
      qux, then qix;
   c. the packages in the packages/ directory of the SDK, in alphanumeric
      order (as if packages/ were appended to the --package-path argument).

Brian: is that correct?


> Since when module from "addon-kit" makes require("events") it still gets
> "events" that is not from "api-utils" but from unrelated package. So we still
> need to narrow down search to a dependencies only.

The CommonJS Packages 1.0 specification <http://wiki.commonjs.org/wiki/Packages/1.0> requires that a CommonJS program or library provide a "dependencies" field in its package descriptor file (package.json) that specifies the "prerequisite packages on which this package depends in order to install and run."

It doesn't, however, specify how module references are resolved (this is left to the Modules 1.1 specification <http://wiki.commonjs.org/wiki/Modules/1.1>), and it doesn't require the field's value to be manually constructed.

The Packages 1.1 proposal <http://wiki.commonjs.org/wiki/Packages/1.1> makes this field optional.  Both versions say the order of options within dependency groups is significant, but such groups are specified as JSON objects, which are unordered.

My sense is that the field is underspecified, and it isn't clear that we should use it at all in its current state, especially since it does not appear to provide an unambiguous reference to a dependent package, like an ID or a URL; it only provides an arbitrary name.

If we do use it, it isn't clear how.  Two options:

A. We generate the value of the field automatically based on the `require` statements in the code.

B. We add a step to the search algorithm, between 3a and 3b, that searches for baz in the packages specified by the field.

Note: if we don't do B, then we should make references in addon-kit to modules in api-utils specify the api-utils package explicitly to prevent problems like the one Irakli is experiencing here.

Comment 11

[Brian Warner [:warner :bwarner]]

(In reply to comment #10)

> If I remember correctly, the algorithm for module resolution is as follows
> (with "foo" being a package whose "bar" module requires the "baz" module):
> 
> 1. If bar requires baz via an explicit package reference, look for baz in
>    the specified package, i.e. require("foo/baz") looks for baz in foo.

yup, although "foo/baz" is ambiguous as to whether it means PACKAGE/MODULE or
SUBDIR/MODULE (leaving the package unspecified, meaning "search through all
of them", such as if you did require("windows/observer") and wanted to get
packages/api-utils/lib/windows/observer.js). The current code tries both, in
that order.

> 2. If bar requires baz via a relative path, look for baz in the directory
> relative to the directory in which bar is located, i.e. require("./baz") looks
> for baz in the same directory as bar.

yup

> 3. Otherwise, i.e. require("baz"), look for baz in the following places:
> 
>    a. the directory in which bar is located;
>    b. the packages in the package path, as specified by the --package-path
>       command-line argument, in the order in which the packages appear in
>       that path, i.e. require("baz") with --package-path=qux:qix looks for
>       baz in qux, then qix;
>    c. the packages in the packages/ directory of the SDK, in alphanumeric
>       order (as if packages/ were appended to the --package-path argument).

not quite.

To search "the directory in which bar is located", you'd do require("./baz"),
and because "./baz" is available, I didn't make require("baz") do the same.
It wasn't clear to me which would be best.. if we have a lot of
code/expectations that unqualified undecorated names ought to be relative,
I'm cool with that. The alternative would be to add "./" to a bunch of our
code's require() statements.

The current behavior for a short require() name (no slashes, no dots) is to
first treat "baz" as a package name, find that package, and look for its main
entry point (except that the "index.js" entry point logic is not actually
implemented yet, so it always skips over this case). Second, it then searches
all packages for a matching module name. The search currently hits all known
packages in arbitrary order (iterating over dictionary keys).

Note that the --package-path appends its (single) value to an internal list
of directories, and packages are found in subdirectories of each element
thereof. So the default behavior acts as if you'd done e.g. "cfx run
--package-path=SDKTOP/packages". So to get packages qux and qix into the
search set, you'd create ~/mypackages/qux and ~/mypackages/qix and add a
--package-path=~/mypackages to the command line.

Searching the package list in random order is certainly wrong. The existing
data structure (the dictionary returned by packaging.build_config() ) doesn't
retain ordering information, probably because the expectation was that each
package will explicitly list its dependency packages in its package.json
under the "dependencies" property (http://wiki.commonjs.org/wiki/Packages/1.1
in the "Package Descriptor File" section), although .dependencies is an
unordered dict too. As best I can tell, the plan was that package A declares
itself to depend upon packages B and C, then any searching done within A
should be limited to looking at B and C (and hopefully the order doesn't
matter).

To really meet that expectation, we'd need add-on authors to add

  "dependencies": {"addon-kit": VERSION},

to their main package.json files, which would be a drag. So we'd kind of like
to silently insert such a dependency in top-level addon packages. Or change
the search behavior.


> The CommonJS Packages 1.0 specification
> <http://wiki.commonjs.org/wiki/Packages/1.0> requires that a CommonJS
> program or library provide a "dependencies" field in its package descriptor
> file (package.json) that specifies the "prerequisite packages on which this
> package depends in order to install and run."

I get the vague sense that .dependencies is mainly for the benefit of
package-installation tools (like "apt" or "yum" or homebrew/macports/Fink) so
that when you ask it to install X, it also installs the Y and Z that X needs
to run.

> My sense is that the field is underspecified, and it isn't clear that we
> should use it at all in its current state, especially since it does not
> appear to provide an unambiguous reference to a dependent package, like an
> ID or a URL; it only provides an arbitrary name.

The namespace for the .dependencies property, and the finer-grained
somewhat-subordinately-scoped namespace of require() calls, are both
ambiguous when considered stronger terms (I'm thinking about the security
model here). They're both used to reference some "standard library" or
community consensus about what code the module author expects their own code
to interact with. When someone in the jetpack world says require("panel"), we
all know what they mean (except for different versions of the SDK, or bugs in
the implementation). When they say require("base32"), we can guess what they
had in mind even if there could be several popular implementations. A GUID,
DNS-like ID, or URL are successively stronger hints (they certainly rule out
a lot of possibilities) but still a large set of parties who can influence
what code you end up running. The far end of this spectrum is to include the
exact code that you want to use, or an effective equivalent like its SHA256
hash, but such overspecification loses the modularity and
abstraction-boundary that is why we use modules in the first place.

But anyways, nailing down the unambiguosity like that is beyond the scope of
this ticket :).

> B. We add a step to the search algorithm, between 3a and 3b, that searches
>    for baz in the packages specified by the field.

Yeah, if ".packages" is useful, we could change the search to only look in
those packages, plus addon-kit for the top-level addon code (if we don't want
to force developers to add .packages just for that).

I can't help thinking that this is all really messy and we should follow
somebody else's spec, instead of hacking out our own. But I'm not yet
convinced that there is a well-specified multiply-implemented spec to follow
yet.

This sort of thing has come up in the Python world, as they've moved from
always-relative import statements to usually-absolute (and changed the
namespace rules around to avoid breakage during the transition). The general
goals are to make sure that dropping a new package into the stdlib (say, by
upgrading your environment to a newer version with a larger stdlib) won't
break any existing code by "stealing" part of the namespace. So if there's
ambiguity, the importer should always search nearby files before reaching for
the standard library or additional packages.

Comment 12

[Brian Warner [:warner :bwarner]]

The false-alarm on bug 654162 made me realize that require("foo") really does need to search the local package first. In that case, reddit-panel/test/test-main.js did require("main") with the intention to get reddit-panel/lib/main.js . It did get it, but if it searches packages in arbitrary order then it could easily have gotten packages/development-mode/lib/main.js by mistake. Importing code from a different section (test/ vs lib/) is a bit squirrly.

Hm, actually, as Irakli has pointed out (and NPM is doing), removing the magic-ness from "lib/" is probably a good idea, as it'd help remove the magic from "test/" too, and could remove this whole confusing "section" concept too. That would mean reddit-panel/test/test-main.js would need to do require("../lib/main.js") to get its main.js, which is a lot more obvious. It would also mean that if main.js wants to get addon-kit/lib/panel.js it should do require("addon-kit/lib/panel"), which is ugly in multiple ways.

Comment 13

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

> To really meet that expectation, we'd need add-on authors to add
> 
>   "dependencies": {"addon-kit": VERSION},
> 
> to their main package.json files, which would be a drag.

Indeed, that would be a drag.  We could ameliorate the pain on addon creation by adding it ourselves in |cfx init| (and also making |cfx init| really just generate skeletal code, so folks actually use it when creating their second and subsquent addons).

But that version string will become out-of-date the moment the developer updates their SDK to a new version.


> So we'd kind of like
> to silently insert such a dependency in top-level addon packages. Or change
> the search behavior.

We could silently insert a dependency on addon-kit, but that seems brittle and error-prone.  Its modules will stick around, as we'll maintain backward compatibility for them, but they won't necessarily live in an addon-kit package.  They might even live in multiple packages (browser/, net/, etc.).


> I get the vague sense that .dependencies is mainly for the benefit of
> package-installation tools (like "apt" or "yum" or homebrew/macports/Fink) so
> that when you ask it to install X, it also installs the Y and Z that X needs
> to run.

That seems right.  Which also means it doesn't make sense for addons, given our static linking model.


> I can't help thinking that this is all really messy and we should follow
> somebody else's spec, instead of hacking out our own. But I'm not yet
> convinced that there is a well-specified multiply-implemented spec to follow
> yet.

I think you're right about that.


Here's what makes sense to me:

The principle is that addon developers shouldn't have to care about declaring package dependencies or know more than is necessary about packages as an additional layer of abstraction between addons and modules.  They should be able to make simple require() calls for modules and have everything just work.

In the most common case, addon developers will use only addon-kit and won't use any third-party libraries.  In the second most common case, they'll use third-party libraries without module name conflicts.  And in the least common case, they'll use third-party libraries with module name conflicts.

For the common cases, then, we neither require .dependencies nor silently hard-code an addon-kit .dependency.  Instead, we rely on a deterministic --package-path search (including the SDK's own packages/ directory by default) to automatically find the right modules (i.e. we continue to do what we're already doing, except that we make it deterministic).

For the least common case, we recommend qualifying require() calls with package names.


For library developers (including ourselves), on the other hand, package dependencies are important.  .dependencies seems inadequate to address their needs, however.  And our implementation doesn't conform to the spec in any case, since our .dependencies is a JSON array of package names, and the Packages spec says it is a "hash" (i.e. JSON object) of package name -> version tuples.

So we remove .dependencies from our implementation, update all require() calls in addon-kit that require api-utils modules to include the package name, and revisit the issue of package .dependencies as part of a broader push for third-party libraries and in conjunction with CommonJS principals.

Comment 14

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

(In reply to comment #13)
> So we remove .dependencies from our implementation, update all require() calls
> in addon-kit that require api-utils modules to include the package name, and
> revisit the issue of package .dependencies as part of a broader push for
> third-party libraries and in conjunction with CommonJS principals.

Hmm, on second thought, although the spec is unclear, our implementation nonconformant, and packages an overcomplicated abstraction... it's late to be making big changes to the way we support packages, especially given that Add-on Builder relies on them.

We're better off making the minimal changes to fix the regression and then blowing up packages after 1.0.

From what I can tell, the minimal changes are:

1. Make package searches deterministic.
2. When .dependencies is specified, and unless overridden by --package-path, restrict package searches to the packages specified by .dependencies.

But:

A. Continue to neither require .dependencies nor silently hard-code an addon-kit dependency.
B. Continue to specify that .dependencies should be an array, even though this violates the unclear spec.

#1 seems doable after 1.0b5; #2 I'm not so sure about.  Adding this to the blocker list for further consideration.  At the very least, we need to figure out the strategy so we know what, if anything, blocks.


(In reply to comment #12)
> Hm, actually, as Irakli has pointed out (and NPM is doing), removing the
> magic-ness from "lib/" is probably a good idea, as it'd help remove the magic
> from "test/" too, and could remove this whole confusing "section" concept too.

Perhaps I misunderstand, but I'd lean toward making "test/" even more magic, as treating it as "just another directory of modules" smacks of overgeneralization.


> That would mean reddit-panel/test/test-main.js would need to do
> require("../lib/main.js") to get its main.js, which is a lot more obvious. It
> would also mean that if main.js wants to get addon-kit/lib/panel.js it should
> do require("addon-kit/lib/panel"), which is ugly in multiple ways.

Indeed.  I don't even want developers to have to specify addon-kit/, much less lib/.

Comment 15

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

> > would also mean that if main.js wants to get addon-kit/lib/panel.js it should
> > do require("addon-kit/lib/panel"), which is ugly in multiple ways.
> 
> Indeed.  I don't even want developers to have to specify addon-kit/, much less
> lib/.

Node packages no longer put their modules in lib folder they just put them into root of the package, so they can be loaded via `require("addon-kit/panel")`

Comment 16

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

Another thing I have been thinking for some time already and found very comfortable for writing code for the web is using full URLs for loading modules.

This puts developers into familiar situation, with a diff that no script injection is required. Also this is much more consistent with design of native modules for ES.next http://wiki.ecmascript.org/doku.php?id=harmony:modules

Also please note that dependencies may be organized so, that only short relative URLs will be used by `require`.

Also I guess it's not a good time to make such a big change.

Comment 17

[Brian Warner [:warner :bwarner]]

irakli: I'm extending your patch in https://github.com/warner/addon-sdk/tree/manifest-654095 , and adding some tests of the linker behavior. Let me know what you think.. I'll be r?-ing this to you sometime today.

Comment 18

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

If Irakli isn't around by then, because it's late in Europe, then let's find someone else to review it, so we can land it and spin 1.0b5rc2 with it today.

Comment 19

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

Thanks Brian, I've tried your changes and it solves the error I was running into.

Comment 20

[Brian Warner [:warner :bwarner]]

Comment on attachment 529486 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/154

Obsoleted by https://github.com/mozilla/addon-sdk/pull/155 , which incorporates the patch.

Comment 21

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

Comment on attachment 529486 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/154

Just a minor issue: one comment in the code, as noted in the pull request, is no longer accurate.

Also, I get this when I reference a module that cannot be found:

--------------------------------------------------------------------------------
(addon-sdk)myk@myk:~/Projects/addon-sdk/foo$ cfx run
Warning: unable to satisfy require(nonexistentmodule) from ModuleInfo [foo lib main] (/mnt/hgfs/myk/Projects/addon-sdk/foo/lib/main.js, /mnt/hgfs/myk/Projects/addon-sdk/foo/README.md)
Using binary at '/home/myk/bin/firefox'.
Using profile at '/tmp/tmpoVU514.mozrunner'.
info: The add-on is running.
error: An exception occurred.
Traceback (most recent call last):
  File "resource://jid0-yrvpapstml8emmwni4knfts9pqq-api-utils-lib/securable-module.js", line 383, in asyncRequire
    return syncRequire(deps);
  File "resource://jid0-yrvpapstml8emmwni4knfts9pqq-api-utils-lib/securable-module.js", line 240, in syncRequire
    throw new Error("Module at "+basePath+" not allowed to require"+"("+module+")");
Error: Module at resource://jid0-yrvpapstml8emmwni4knfts9pqq-foo-lib/main.js not allowed to require(nonexistentmodule)
FAIL
Total time: 1.742683 seconds
Program terminated unsuccessfully.
--------------------------------------------------------------------------------

The messages "unable to satisfy" and "not allowed to require" seem suboptimal in this case.  It'd be nice if this said something like "can't find module 'nonexistentmodule' in [list of places we looked]".

But that can be a separate bug.

Comment 22

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

Erm, and a=myk for commission during freeze.

Comment 23

[Brian Warner [:warner :bwarner]]

Landed in https://github.com/mozilla/addon-sdk/commit/d800769b86bd9d4b681d351450d6c135f53957c2

Bug 654615 filed about the error/warning message improvements.

Comment 24

[Brian Warner [:warner :bwarner]]

removing the "checkin-needed" keyword since I landed it.. I *think* that's talking about trunk, but please bring it back if I guessed wrong.

Comment 25

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

That's right, "checkin-needed" is for trunk only.  And you can resolve fixed once the fix has landed on the trunk, too.

Cherry-picked commit to 1.0b5 branch:

https://github.com/mozilla/addon-sdk/commit/ff546a79bf2880b6105c1020bc1c4f9ddb8e1b9a