591525 - identify dependent modules by relative path

Status: RESOLVED FIXED

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

Description

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

Currently, the module dependency graph manifest added in bug 572020 identifies dependent modules by name.  Manifest consumers, like a tool for helping reviewers review addons, can then determine the module file in an addon XPI bundle to which a given name refers via an algorithm like that implemented by the module loader's module name resolver.

It would be better for the manifest to identify dependent modules by relative path rather than name, so that manifest consumers like the aforementioned review tool don't have to reimplement the resolver's algorithm, which risks the introduction of subtle bugs that a malicious addon could exploit to trick reviewers into thinking the addon uses a different module to perform some task than the one it actually uses.

Comment 1

[Brian Warner [:warner :bwarner]]

Attachment: first step: track dependencies as a mapping, not an array, so there will be a place for the pathname

Comment 2

[Brian Warner [:warner :bwarner]]

Attachment: check loaded path against manifest

this adds the second step: at runtime, compare the loaded path (i.e. the resource: URL from which the .js module was loaded) against the ".filename" data from the manifest.

This currently fails, because the link-time code (in manifest.py) is only putting a fake pathname in the manifest. The next step is to put enough package-path search code in manifest.py to figure out the right filename/URL for each module, to put real values into the manifest instead of the fake ones.

Comment 3

[Brian Warner [:warner :bwarner]]

Attachment: check loaded path against manifest

ok, this one passes tests. I renamed the "options" argument passed to allowEval and called it "module_info" instead, since there are an awful lot of "options" flying around. I cache the module_info (which includes the resource: URL from which the file was loaded) because the lookup only occurs when the module is first loaded (and allowEval called), but the manifest check needs to happen every time the module is required (including the second and later times when allowEval is skipped because the module is already cached).

This code constructs the expected resource: URL at link time (basically only nailing down which package we think will provide it) and adds it to the manifest. This requires a second pass over the manifest, since the data from the first pass is used to perform the path search. I'd like to enhance it to record a hash of the module's contents too, even if the runtime loader won't be using that information for a while yet.

Comment 4

[Brian Warner [:warner :bwarner]]

oh, and note that this patch still triggers some warnings, probably because
my second manifest pass isn't looking at all the appropriate files. The
(parent-package, parent-module, child-module) of the warnings are:

NOT FOUND test-harness harness nsjetpack
NOT FOUND test-harness harness parent-loader
NOT FOUND jetpack-core unit-test parent-loader
NOT FOUND jetpack-core content/symbiont ./loader
NOT FOUND jetpack-core content/symbiont ./worker

i.e. jetpack-core/lib/content/symbiont.js did a require("./worker") which
didn't match anything in the manifest. For this case, I think that I don't
understand how CommonJS require() names are supposed to be interpreted.. I
guess "./worker" means that it wants to find a worker.js in
jetpack-core/lib/worker.js? Or maybe jetpack-core/lib/content/worker.js?

Comment 5

[Atul Varma [:atul]]

Yeah, the "./" and "../" paths are relative to the "directory" that the calling module is in, so when jetpack-core/lib/content/symbiont.js requires "./worker", it's going to get jetpack-core/lib/content/worker.js.

Comment 6

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

CommonJS has also changed the intended interpretation of require() names recently, including requiring them to include the names of the packages to which they belong.  Irakli knows more; cc:ing him.

Comment 7

[Atul Varma [:atul]]

Comment on attachment 478145 [details] [diff] [review]
check loaded path against manifest

If cuddlefish.manifest, you have:

> +            if found:
> +                # now compute the zipfile name (actually the URL)
> +                (p1,r1) = found
> +                url = "resource://%s%s-lib/%s.js" % (jid_prefix, p1, r1)
> +                requires[reqname]["filename"] = url

But the url's resources won't always have the word 'lib' after them. We may want to move the URL-generation code come from the same place that cuddlefish.packaging's generate_build_for_target() uses to determine the resource URI of a library to ensure correct behavior and not violate DRY. Specifically, see the add_section_to_build() function defined in the closure of generate_build_for_target().

Other than that, looks good!

Comment 8

[Brian Warner [:warner :bwarner]]

Ok, so my understanding of our current search-path rules (using, as an
example, a "package path" of A,B,C, and a require(X) statement found in a
module inside package B) is as follows:

* if X starts with "./" or "../", then we only look in package B (the
  module's "home package"). If X looks like "./FOO", we're looking for a
  FOO.js that lives next to the module doing the require(). When X looks like
  "../FOO", we're looking for a FOO.js in the parent directory of the module
  doing the require()
* if X does not start with ".", then we search package A, then B, then C. If
  X looks like "FOO", we're looking for A/lib/FOO.js, or B/lib/FOO.js, or
  C/lib/FOO.js . If X looks like "bar/FOO", we're looking for
  {A,B,C}/lib/bar/FOO.js .

Does that seem right?

It sounds like CommonJS has fancier rules than these (which allow require()
statements that call for specific packages), but that we don't currently
implement those. So my goal is going to be to put code in manifest.py that
matches our current run-time loader's behavior. When we fix the runtime to
implement fancier rules, we'll need to update the linker (in manifest.py) to
match. I'll follow Irakli's lead on that front.

Comment 9

[Brian Warner [:warner :bwarner]]

http://github.com/mozillalabs/jetpack-sdk/pull/10  is the new patch, ready for review. I don't *think* it causes any new tests to break, but it's hard to tell, because current jetpack-sdk trunk fails so many tests against both FF4.0b6 and current minefield. But when I based this patch against jetpack-sdk from a week ago, it passed all tests against 4.0b6. YMMV.

Comment 10

[Atul Varma [:atul]]

By the way, the hack to upload a "patch" that represents the github pull request can be done by uploading an HTML file with the following contents:

  <!DOCTYPE html>
  <meta charset="utf-8">
  <meta http-equiv="refresh" content="5;http://github.com/mozillalabs/jetpack-sdk/pull/7">
  <title>Bugzilla Code Review</title>
  <p>You can review this patch at <a href="http://github.com/mozillalabs/jetpack-sdk/pull/7">http://github.com/mozillalabs/jetpack-sdk/pull/7</a>, or wait 5 seconds to be redirected there automatically.</p>

Just Replace all occurrences of "/7" with the ID of your pull request, which is "/10" in this case.

The reason this hack is particularly nice is because it shows up as a pending review on my bugzilla dashboard, so I won't forget about it.

Comment 11

[Atul Varma [:atul]]

Attachment: Warner's github pull request for patch

I'm requesting my own review, which is kind of weird, but whatever.

Comment 12

[Atul Varma [:atul]]

Comment on attachment 481862 [details]
Warner's github pull request for patch

I dig it! See my comments on the github pull request for more info, and let me know if you think we should file any follow-up bugs based on my review.

Comment 13

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

With the patches in bug 603849 and bug 603914, almost all tests pass on trunk nightlies, making it much easier to see if this breaks anything, so I'm comfortable landing it.

Comment 14

[Brian Warner [:warner :bwarner]]

Ok, landed: http://hg.mozilla.org/labs/jetpack-sdk/rev/6a0e49e1abc9

I hope it doesn't break anything. SDK-trunk and the latest nightly seems to completely hang during tests (probably bug 603849), so I wasn't able to verify anything beyond a basic 'cfx testcfx' (which only exercises the python bits) on trunk.

I've opened bug 604089 for the cosmetic/style issues raised above.

Comment 15

[Dietrich Ayala (:dietrich)]

I have a test i'm working on that uses Cu.import. As of today, I get:

error: An exception occurred.
Traceback (most recent call last):
  File "resource://testpkgs-jetpack-core-lib/xpcom.js", line 41, in 
TypeError: Cu.import is not a function

Would that be related to this bug?

Comment 16

[Dietrich Ayala (:dietrich)]

I read the error incorrectly - it's not my call to Cu.import that's failing, it's the xpcom module's call to Cu.import.

Comment 17

[Dietrich Ayala (:dietrich)]

Anything that uses xpcom module is broken. Filed bug 604362 for it.

No idea if it's related to this or not, just commented here initially because it looked like a candidate.

Comment 18

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

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".