Don't abuse readURL to detect the existence of a script

RESOLVED FIXED in Firefox 50

Status

Add-on SDK
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

({addon-compat})

unspecified
mozilla52
addon-compat
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

Comment hidden (empty)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Summary: Don't fall readURL to detect the existence of a script → Don't abuse readURL to detect the existence of a script

Comment 3

2 years ago
mozreview-review
Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

https://reviewboard.mozilla.org/r/85126/#review83814

Looks good, thanks for all these cleanups.

We should have introduced startsWith sooner!!

::: addon-sdk/source/lib/toolkit/loader.js:491
(Diff revision 1)
>      }
> -    try {
> -      let tmpPath = path + '/index.js';
> -      readURI(tmpPath);
> -      return tmpPath;
> -    } catch (e) {}
> +  } catch (e) {}

What was that? Just duplicated for no reason?!

::: addon-sdk/source/lib/toolkit/loader.js:729
(Diff revision 1)
>          // If the override is for x -> y,
>          // then using require("x/lib/z") to get reqire("y/lib/z")
>          // should also work
> -        if (id == key || (id.substr(0, key.length + 1) == (key + "/"))) {
> +        if (id == key || id.startsWith(key + "/")) {
>            id = overrides[key] + id.substr(key.length);
> -          id = id.replace(/^[\.\/]+/, "./");
> +          id = id.replace(/^([\.\/]+)?/, "");

I don't quite follow why you need the additional "(  )?", but may be my regexp is rusty...
It seems useless given that we are using `replace`.
Attachment #8800095 - Flags: review?(poirot.alex) → review+

Comment 4

2 years ago
mozreview-review
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review83822

This shouldn't work for unpackaged addons, like addons loaded from sources via about:debugging?
Also it is quite complex (the ziprequest, the caches, the observer notifications...). Did you had any alternative ideas?
For ex, Would this be enough?
http://searchfox.org/mozilla-central/source/services/sync/tps/extensions/mozmill/resource/stdlib/securable-module.js#318
It still uses a channel but not reading its full content...

Also, did you tried to run a test pilot addon with all these patches applied?
Given the complexity of the loader I would be more confident knowing a non-naive addon still works fine!
Attachment #8800096 - Flags: review?(poirot.alex)
It does work with packaged add-ons, yes. I didn't test with Test Pilot, but I did test with GeckoProfiler. Unfortunately, though, as far as I can tell, we don't actually have automated tests for this :(

Unfortunately, the complexity is necessary, due to the huge number of files and paths the node resolver looks for. My initial version (which was a bit simpler, but not as simple as just using a channel) gave about a 15% speed-up on the Jetpack tests. This version gives about a 28% speed-up. On the entire test run, not just the module loading...
(Assignee)

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

https://reviewboard.mozilla.org/r/85126/#review83814

> What was that? Just duplicated for no reason?!

Yeah... I really have no idea why, but it definitely wasn't necessary

> I don't quite follow why you need the additional "(  )?", but may be my regexp is rusty...
> It seems useless given that we are using `replace`.

You're right, it's not necessary.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Updated to run resolution tests with both packed and unpacked add-ons, and with all relevant URL types.
Comment hidden (mozreview-request)
Also removed the node resolver cache, since it doesn't make a difference anymore.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

https://reviewboard.mozilla.org/r/85124/#review84200

::: addon-sdk/source/lib/toolkit/loader.js:335
(Diff revision 3)
> +function checkUrlExists(url) {
> +  if (!/\.(?:js|json)$/.test(url))
> +    url = addTrailingSlash(url);
> +
> +  let baseURL = getBaseURL(url);
> +

I don't know what was your simplier version, but how different the performances would be if you do something like this here:
  var channel = NetUtil.newChannel({
    uri: Services.io.newURI(url, null, null),
    loadUsingSystemPrincipal: true
  });
  try {
    channel.open2().close();
    return true;
  } catch (e) {
    return false;
  }
And cache this result in a DefaultMap?
The complexity that disturbs me here is that we have some code specific to each scheme.

Comment 13

2 years ago
mozreview-review
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review84118

::: addon-sdk/source/lib/toolkit/loader.js:371
(Diff revisions 1 - 3)
>  function join(...paths) {
>    let joined = pathJoin(...paths);
>    let resolved = normalize(joined);
>  
> +  // Make sure that resource: and jar: URLs have the necessary trailing slash.
> +  resolved = resolved.replace(/^(?:jar:[^!]+!|resource:\/[^\/]+)$/, "$&/");

This replace isn't clear. It is hard to guess what it tries to address.

Is that because normalize also strip trailing slash?
Why is that specific to only jar and resource?
Why does is replace resource:/foo to resource:/foo/
as well as resource:/foo.html to resource:/foo.html/?
Couldn't it be done using startsWith("resource") && !endsWith("/")? Or call the existing addTrailingSlashes?
Attachment #8800096 - Flags: review?(poirot.alex)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review84228

::: addon-sdk/source/test/test-native-loader.js:1
(Diff revision 3)
>  /* This Source Code Form is subject to the terms of the Mozilla Public

Oh, and I tried to run this test locally without much success. That may be helpful to put a comment on how to do that in the test itself.
Duplicate of this bug: 1036625
(Assignee)

Comment 16

2 years ago
mozreview-review-reply
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review84118

> This replace isn't clear. It is hard to guess what it tries to address.
> 
> Is that because normalize also strip trailing slash?
> Why is that specific to only jar and resource?
> Why does is replace resource:/foo to resource:/foo/
> as well as resource:/foo.html to resource:/foo.html/?
> Couldn't it be done using startsWith("resource") && !endsWith("/")? Or call the existing addTrailingSlashes?

Yes, normalize strips the trailing slash, and sometimes produces invalid URLs like "resource://foo" and "jar:file:///foo/!", which are illegal. We don't need the URLs to end with `/`, but we do need them to at least always have a path component.
(Assignee)

Comment 17

2 years ago
mozreview-review-reply
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review84228

> Oh, and I tried to run this test locally without much success. That may be helpful to put a comment on how to do that in the test itself.

I wound up doing it by commenting out all of the other tests in `jetpack-package.ini` and running `mach mochitest --flavor=mochitest-jetpack-package --disable-e10s addon-sdk/source/test/` but that's the same for any test in this suite.
Mossop, since you apparently tried to tackle this before I did, do you have any opinions on which approach we should prefer?
Flags: needinfo?(poirot.alex)
Flags: needinfo?(dtownsend)
(In reply to Alexandre Poirot [:ochameau] from comment #12)
> Comment on attachment 8800095 [details]
> Bug 1309350: Part 1 - Remove dead code and clean up cruft.
> 
> https://reviewboard.mozilla.org/r/85124/#review84200
> 
> ::: addon-sdk/source/lib/toolkit/loader.js:335
> (Diff revision 3)
> > +function checkUrlExists(url) {
> > +  if (!/\.(?:js|json)$/.test(url))
> > +    url = addTrailingSlash(url);
> > +
> > +  let baseURL = getBaseURL(url);
> > +
> 
> I don't know what was your simplier version, but how different the
> performances would be if you do something like this here:
>   var channel = NetUtil.newChannel({
>     uri: Services.io.newURI(url, null, null),
>     loadUsingSystemPrincipal: true
>   });
>   try {
>     channel.open2().close();
>     return true;
>   } catch (e) {
>     return false;
>   }
> And cache this result in a DefaultMap?
> The complexity that disturbs me here is that we have some code specific to
> each scheme.

Manually copying my comment from mozreview, since for some reason it didn't wind up in the bug:

    OK, I tried this, and with caching, the test run is only slightly (~2%) slower than with the other version. I'm still leaning toward the other approach for a few reasons, though:

    1) That's still 2% of the entire test suite run, not just of the module loading overhead.

    2) The test suite benefits much more from this kind of caching than an extension would at cold startup. That's especially true for the common case of extensions stored as packed XPIs, where every URL existence check is reduced to a simple Set lookup.

    3) This approach doesn't work for testing the existence of directories, which means that we can't skip the lookups in node_modules directories that we know don't exist, and have to do an uncached check for each module in each parent directory of the current module whenever it's required.

    4) The extra overhead is going to hit most users far harder than it hits my test system, with its gigabytes of RAM, memory-cached filesystem contents, fast SSD, and overpowered CPU.
When I tackled this in bug 1036625 I resolved urls down to an nsIFileURL or nsIJARURI and used the file and zip APIs to check for file existence. My suspicion is that that is the fastest since attempting to open a channel to a url is going to have to do all that as a minimum as well as seeking and setting up a reading stream to the source (unless we lazily do that)
Flags: needinfo?(dtownsend)
Comment hidden (mozreview-request)
This version simplifies things a bit, but still uses the direct URL resolution for now. I'm open to switching to the channel-based approach if necessary, but I still think the slightly increased complexity of this approach is worth it for the additional efficiency.
Keywords: leave-open

Comment 25

2 years ago
mozreview-review
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review85008

Ok, let's go with the fastest option. I just have a bunch of nits.

Thanks for maintaining this orphan code!

::: addon-sdk/source/lib/toolkit/loader.js:241
(Diff revision 4)
> +   *
> +   * @returns {Set<string>}
> +   */
> +  getZipFileContents(uri, baseURL) {
> +    let file = uri.JARFile.QueryInterface(Ci.nsIFileURL).file;
> +    let basePath = addTrailingSlash(uri.JAREntry).slice(1);

nit: a comment saying that we remove the trailing slash would help. Because calling addTrailingSlash to do that is hard to follow.

::: addon-sdk/source/lib/toolkit/loader.js:243
(Diff revision 4)
> +   */
> +  getZipFileContents(uri, baseURL) {
> +    let file = uri.JARFile.QueryInterface(Ci.nsIFileURL).file;
> +    let basePath = addTrailingSlash(uri.JAREntry).slice(1);
> +
> +    let enumerator = zipCache.getZip(file).findEntries("(*.js|*.json|*/)");

Shouldn't we rather do the opposite?
findEntries(basePath) and then iterate to filter the .js, .json and directories?
We might even be able to come up with a search string for findEntries that doesn't need any filter?
basePath + "/*.json|" + basePath + "/*.js| "+basePath + "/*/"
?

::: addon-sdk/source/lib/toolkit/loader.js:261
(Diff revision 4)
> +
> +  zipContentsCache: new DefaultMap(baseURL => {
> +    let uri = NetUtil.newURI(baseURL);
> +
> +    if (baseURL.startsWith("resource:"))
> +      uri = NetUtil.newURI(resProto.resolveURI(uri));

nit: here and elsewhere, sdk codebase uses braces even for single line if blocks.

::: addon-sdk/source/lib/toolkit/loader.js:279
(Diff revision 4)
> +    } catch (e) {
> +      return false;
> +    }
> +  }),
> +
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsISupportsWeakReference]),

nit: Add an empty line between QueryInterface and observe

::: addon-sdk/source/lib/toolkit/loader.js:316
(Diff revision 4)
> +   * @returns {boolean}
> +   */
> +  exists(url) {
> +    if (!/\.(?:js|json)$/.test(url))
> +      url = addTrailingSlash(url);
> +

nit: You may move create of nsIURI in exists() (instead of zipContentsCache/filesCaches to create only one such object. Because here you are instanciating it twice in each.
I may also be correct to only call zipContentsCache only if you can query it to nsIJARURI from here.
Same for fileCaches, only call it if it queries to nsIFileURL?

::: addon-sdk/source/lib/toolkit/loader.js:355
(Diff revision 4)
> -function join(...paths) {
> -  let joined = pathJoin(...paths);
> -  let resolved = normalize(joined);
> -
> -  // OS.File `normalize` strips out any additional slashes breaking URIs like
> -  // `resource://`, `resource:///`, `chrome://` or `file:///`, so we work
> +function join(base, ...paths) {
> +  // If this is an absolute URL, we need to normalize only the path portion,
> +  // or we wind up stripping too many slashes and producing invalid URLs.
> +  let match = /^((?:resource|file|chrome)\:\/\/[^\/]*|jar:[^!]+!)(.*)/.exec(base);
> +  if (match)
> +    return match[1] + normalize(pathJoin(match[2], ...paths));

Thanks, it looks clearer.
(do not forget about the braces for all single line if blocks!)
Attachment #8800096 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 26

2 years ago
mozreview-review-reply
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review85008

> Shouldn't we rather do the opposite?
> findEntries(basePath) and then iterate to filter the .js, .json and directories?
> We might even be able to come up with a search string for findEntries that doesn't need any filter?
> basePath + "/*.json|" + basePath + "/*.js| "+basePath + "/*/"
> ?

I've done that in the past[1], but dealing with escaping metacharacters makes the code a lot more complicated, so I decided to go with the simpler route here. Also, the normal case for this is a `resource:` URI that points to the root of an XPI, so we should probably get better filtering by extension than by base path.

[1]: http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/toolkit/components/extensions/Extension.jsm#921-924

> nit: here and elsewhere, sdk codebase uses braces even for single line if blocks.

Hm. I'd normally use braces, but this module (and most of the SDK code I've seen recently) seemed to pretty consistently omit them. But I'm happy to add them in code this touches anyway.

> nit: You may move create of nsIURI in exists() (instead of zipContentsCache/filesCaches to create only one such object. Because here you are instanciating it twice in each.
> I may also be correct to only call zipContentsCache only if you can query it to nsIJARURI from here.
> Same for fileCaches, only call it if it queries to nsIFileURL?

I did that initially, but for the normal case of a `resource:` URL that points to a `jar:` URL, this version doesn't create a `nsIURI` at all for most calls. For `jar:` URLs, we only create one once the JAR is cached. For `file:` URLs that aren't cached, we create two, but that's the (hopefully least common) case where we have the most IO overhead anyway, so it's relatively less important.

And, as for only querying `zipContentsCache` even if we can't query to `nsIJARURI` here, the benefit is that we cache the failure for file-backed `resource:` URLs, and save a fair bit of XPConnect overhead in future checks.
(Assignee)

Comment 27

2 years ago
mozreview-review-reply
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review85008

> I did that initially, but for the normal case of a `resource:` URL that points to a `jar:` URL, this version doesn't create a `nsIURI` at all for most calls. For `jar:` URLs, we only create one once the JAR is cached. For `file:` URLs that aren't cached, we create two, but that's the (hopefully least common) case where we have the most IO overhead anyway, so it's relatively less important.
> 
> And, as for only querying `zipContentsCache` even if we can't query to `nsIJARURI` here, the benefit is that we cache the failure for file-backed `resource:` URLs, and save a fair bit of XPConnect overhead in future checks.

(I'll add some comments about this)
Keywords: addon-compat
Flags: needinfo?(poirot.alex)
Formatted:

(In reply to Kris Maglione [:kmag] from comment #27)
> Comment on attachment 8800096 [details]
> Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.
> 
> https://reviewboard.mozilla.org/r/85128/#review85008
> 
>> I did that initially, but for the normal case of a `resource:` URL that points to a `jar:` URL, this
>> version doesn't create a `nsIURI` at all for most calls. For `jar:` URLs, we only create one once the JAR 
>> is cached. For `file:` URLs that aren't cached, we create two, but that's the (hopefully least common) 
>> case where we have the most IO overhead anyway, so it's relatively less important.
>> 
>> And, as for only querying `zipContentsCache` even if we can't query to `nsIJARURI` here, the benefit is
>> that we cache the failure for file-backed `resource:` URLs, and save a fair bit of XPConnect overhead in 
>> future checks.
> 
> (I'll add some comments about this)

Comment 29

2 years ago
mozreview-review
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review86322

There is already code in the build system to produce zip files. The code is intelligent and does things like not zlib compressing entries if they are larger than their input.

Unfortunately, I don't think we have an easy mechanism to hook up this code directly via moz.build, so you'll need a small proxy script.

::: addon-sdk/source/test/fixtures/create_xpi.py:7
(Diff revision 4)
> +import os.path
> +import zipfile
> +
> +
> +def main(output, input_dir):
> +    output.mode = 'rb'
> +
> +    with zipfile.ZipFile(output, 'w') as output_zip:
> +        base_dir = os.path.abspath(input_dir)
> +
> +        for (dirpath, dirnames, filenames) in os.walk(base_dir):
> +            for filename in filenames:
> +                path = os.path.join(dirpath, filename)
> +                relpath = os.path.relpath(path, base_dir).replace('\\', '/')
> +
> +                output_zip.write(path, relpath)

Replace this with:


# TODO replace this script with a direct Python action invocation
from mozbuild.action.zip import main as create_zip

def main(output, input_dir):
    return create_zip(['-C', input_dir, output, '**'])


This code will generate a well-formed and deterministic zip file.

::: addon-sdk/source/test/fixtures/moz.build:18
(Diff revision 4)
> +    file_ = GENERATED_FILES[xpi]
> +    file_.script = 'create_xpi.py'
> +    file_.inputs = [fixture]

Please don't use a trailing underscore. Just use `f` as the variable name.
Attachment #8800096 - Flags: review-
Comment hidden (mozreview-request)
> Unfortunately, I don't think we have an easy mechanism to hook up this code directly via moz.build, so you'll need a small proxy script.

That script already exists doesn't it? (mozbuild.action.zip)
(In reply to Mike Hommey [:glandium] from comment #31)
> > Unfortunately, I don't think we have an easy mechanism to hook up this code directly via moz.build, so you'll need a small proxy script.
> 
> That script already exists doesn't it? (mozbuild.action.zip)

Except scripts used as $(call py_action) have a different "calling convention" from GENERATED_FILES[x].script :/

Comment 33

2 years ago
mozreview-review
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

https://reviewboard.mozilla.org/r/85128/#review86594
Attachment #8800096 - Flags: review?(gps) → review+

Comment 34

2 years ago
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a9a143d2179a
Part 2 - Speed up synchronous resolution of module paths. r=gps,ochameau
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

Approval Request Comment
[Feature/regressing bug #]: Bug 935109

[User impact if declined]:

The current version of this code has severe performance issues, particularly at startup. With the rollout of e10s to users of add-ons, the problem becomes much worse, since it affects both main process and content process startup time.

[Describe test coverage new/current, TreeHerder]:

The module resolution functionality affected by these changes is covered by existing tests, but those tests only test under one (non-common) configuration. This change adds tests for the other, more common configurations.

[Risks and why]:

Moderate. This code has not been well maintained, and required significant refactoring to implement these changes. It's possible that some complex add-ons may rely on obscure quirks in its behavior that are not well-understood. QA is currently testing with popular and complex add-ons, which should reduce the likelihood that anything has been missed.

[String/UUID change made/needed]: None.
Attachment #8800096 - Flags: approval-mozilla-beta?
Attachment #8800096 - Flags: approval-mozilla-aurora?
Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

Approval Request Comment: See comment 35
Attachment #8800095 - Flags: approval-mozilla-beta?
Attachment #8800095 - Flags: approval-mozilla-aurora?
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → fixed
Target Milestone: --- → mozilla52
Comment on attachment 8800096 [details]
Bug 1309350: Part 2 - Speed up synchronous resolution of module paths.

I reviewed the patches attached to both of those bugs and I do *not* feel comfortable uplifting them to 50.0b10 especially given that QA is still ongoing. If rejecting these uplifts to Beta50 means we have fewer add-ons getting white listed in 50 then (sorry!) but that is a potential situation we need to prepare for.

The fact that this code has not stabilized on pre-beta channels sufficiently is a big concern. The risk mentioned in bug 1309350 is medium and the patches landed in Nightly yesterday which is clearly not enough bake time to minimize risk to quality when uplifting so late in the Beta cycle.
Attachment #8800096 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Updated

2 years ago
status-firefox50: affected → wontfix
Attachment #8800095 - Attachment is obsolete: false
Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

Fix a performance issue at startup. I'd like to take it in 51 aurora and see if any issues happen.
Attachment #8800095 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

2 years ago
Attachment #8800096 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Note: The first part of this needs a minor change for beta, since separate lexical scopes for for-of iterations landed in 51: https://hg.mozilla.org/try/rev/e7e1d1f4fb2284c9d8125beb7f0547d0643f8a4e
status-firefox50: wontfix → affected
Comment on attachment 8800095 [details]
Bug 1309350: Part 1 - Remove dead code and clean up cruft.

There was a long discussion on the benefits, risks and how to mitigate those risks. We have decided to take this in 50, Beta+
Attachment #8800095 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

2 years ago
Attachment #8800096 - Flags: approval-mozilla-beta- → approval-mozilla-beta+

Updated

2 years ago
Depends on: 1315547
You need to log in before you can comment on or make changes to this bug.