892100 - Script actor's source loading error reporting broken

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[:Gijs (he/him)]

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)

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: v1

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.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

https://hg.mozilla.org/integration/fx-team/rev/954a86d88d25

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/954a86d88d25