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

RESOLVED FIXED in mozilla36

Status

()

Core
XPConnect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: WeirdAl, Assigned: WeirdAl)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Created attachment 8515423 [details] [diff] [review]
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.
Attachment #8515423 - Flags: review?(bobbyholley)
(Assignee)

Updated

4 years ago
Version: 26 Branch → Trunk
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.
Attachment #8515423 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 2

4 years ago
Created attachment 8516490 [details] [diff] [review]
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...
Attachment #8516490 - Flags: review+
Attachment #8516490 - Flags: checkin?(bobbyholley)
(Assignee)

Comment 3

4 years ago
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 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.
Attachment #8516490 - Flags: checkin?(bobbyholley) → review+
(Assignee)

Comment 5

4 years ago
Created attachment 8519494 [details] [diff] [review]
patch for try run, checkin
Attachment #8516490 - Attachment is obsolete: true
Pushed to try, hopefully all it needed was xpcshell on all platforms:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b3baeb551cb7

Updated

4 years ago
Blocks: 1097444
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed75029de4b4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36

Comment 9

4 years ago
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
Duplicate of this bug: 1098636
You need to log in before you can comment on or make changes to this bug.