1092477 - Let the subscript Loader report any URL it fails on, when it can

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Alex Vincent [:WeirdAl]]

Attachment: subscript-url.diff

It's hard to figure out which file the subscript loader failed to load.  The error message doesn't tell us which one.

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 8515423 [details] [diff] [review]
subscript-url.diff

Review of attachment 8515423 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a simple mochitest-chrome test for this (I think you should be able to do it by just importing a bogus JSM URI, right?), since I might otherwise break this when refactoring this code. r=me, and thanks for fixing this.

::: addon-sdk/source/lib/toolkit/loader.js
@@ +325,5 @@
>      // Note that `String(error)` where error is from subscript loader does
>      // not puts `:` after `"Error"` unlike regular errors thrown by JS code.
>      // If there is a JS stack then this error has already been handled by an
>      // inner module load.
> +    if (String(error) === "Error opening input stream (invalid filename?): " + module.uri) {

Can you please turn this into a RegExp?

if (/^Error opening input stream.*/.test(String(error))) {
....
}

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +96,5 @@
> +{
> +    if (!uri)
> +        return ReportError(cx, origMsg);
> +
> +    nsCString spec;

This should be nsAutoCString so that it can be stack-allocated, right?

@@ +101,5 @@
> +    nsresult rv = uri->GetSpec(spec);
> +    if (NS_FAILED(rv))
> +        spec.Assign("(unknown)");
> +
> +    nsCString msg(origMsg);

Same.

::: testing/mochitest/browser-test.js
@@ +623,5 @@
>      } catch (ex) {
>        // Ignore if no head.js exists, but report all other errors.  Note this
>        // will also ignore an existing head.js attempting to import a missing
>        // module - see bug 755558 for why this strategy is preferred anyway.
> +      if (ex.toString() != 'Error opening input stream (invalid filename?): ' + headPath) {

Same here.

::: toolkit/devtools/worker-loader.js
@@ +152,5 @@
>      let sandbox = createSandbox(url, prototype);
>      try {
>        loadInSandbox(url, sandbox);
>      } catch (error) {
> +      if (String(error) === "Error opening input stream (invalid filename?): " + url) {

And here.

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: patch for checkin

I checked the full message of the exception in the requested chrome mochitest.

A couple of minor concerns:
(1) I was expecting exn.message to be defined; it wasn't.
(2) There's a number of other error conditions the subscript loader throws which no tests exist for...

Comment 3

[Alex Vincent [:WeirdAl]]

Comment on attachment 8516490 [details] [diff] [review]
patch for checkin

Review of attachment 8516490 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/tests/chrome/test_bug1092477.xul
@@ +4,5 @@
> +                 type="text/css"?>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=1092477
> +-->
> +<window title="Mozilla Bug 654370"

Whoops.  I forgot to change the title.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8516490 [details] [diff] [review]
patch for checkin

Review of attachment 8516490 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Please update the bug number in the test (as you noted) and then do a try push (with the link in the bug), then use the checkin-needed keyword to have a sheriff land it for  you.

Comment 5

[Alex Vincent [:WeirdAl]]

Attachment: patch for try run, checkin

Comment 6

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

Pushed to try, hopefully all it needed was xpcshell on all platforms:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b3baeb551cb7

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed75029de4b4

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/ed75029de4b4

Comment 9

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/0f59089f67607145d78dd2ac49f19caf703cf98e
Bug 1092477 - Let the subscript Loader report any URL it fails on, when it can. r=bholley