Closed Bug 892100 Opened 6 years ago Closed 6 years ago

Script actor's source loading error reporting broken

Categories

(DevTools :: Debugger, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: Gijs, Assigned: fitzgen)

Details

(Whiteboard: [chrome-debug])

Attachments

(1 file)

There are a few issues with it. First of all:

function reportError(aError, aPrefix="") {
  let msg = prefix + aError.message + ":\n" + aError.stack;

is unlikely to work. It should be aPrefix. Maybe jimb has already pushed a fix for this, as it's pretty trivial.

However, even correcting that, the errors I was seeing (for loading testpilot scripts; I'll file a separate bug about the actual issue in a second) from here looked like this:

[22:53:13.642] Got an exception during SA_onSource: undefined:undefined @ resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/actors/script.js:2928

I think the prefix should include the source we're trying to load. It also should have a stack; I'm not sure why those don't seem to exist on the object it gets passed, but hopefully someone else knows.

(STR to see some of this: open the browser debugger, in the scripts list, navigate to the testpilot script and click it, then check the browser console in the debuggee)
Whiteboard: [chrome-debug]
Woops, this is my fault. Sorry about that.

Problem is that we are rejecting deferreds with strings in `fetch` and not with Error objects.

Can throw up a patch in a second.
Assignee: nobody → nfitzgerald
Attached patch v1Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=ba7b12783c45

Haven't verified the fix locally (still building).

Not sure how to write a test for this since our tests fail if there are any Cu.reportErrors.
Attachment #773604 - Flags: review?(past)
Comment on attachment 773604 [details] [diff] [review]
v1

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

I don't think a test is absolutely necessary. Using dbg_assert should be OK.

::: toolkit/devtools/server/actors/script.js
@@ +2923,5 @@
>   * @param String aPrefix
>   *        An optional prefix for the reported error message.
>   */
>  function reportError(aError, aPrefix="") {
> +  let msg = aPrefix + aError.message + ":\n" + aError.stack;

If you add a dbg_assert here to make sure aError is an Error object, my patch in bug 832982 will make sure tests fail when reportError gets called with a string.
Attachment #773604 - Flags: review?(past) → review+
https://hg.mozilla.org/integration/fx-team/rev/954a86d88d25
Whiteboard: [chrome-debug] → [chrome-debug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/954a86d88d25
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [chrome-debug][fixed-in-fx-team] → [chrome-debug]
Target Milestone: --- → Firefox 25
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.