642419 - Errors in content scripts don't get displayed anywhere

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[(not currently active) Ted Mielczarek]

Currently if you have an error in a content script, you get no notification of it. Nothing in the error console, nothing on the console in a debug build (IIRC), your code just doesn't work. This makes touching the SpecialPowers code extremely frustrating, as it's very easy to make a syntax error and then the tests just fail with "SpecialPowers is not defined".

We should forward message manager script errors to the error console. (Ideally that would work in the OOP scenario as well, but I'd settle for it working in-process to start.)

Comment 1

[(not currently active) Ted Mielczarek]

I looked for a dupe, because I swore I filed this bug the last time I hit it, but I couldn't find one.

Comment 2

[Josh Matthews [:jdm]]

I suspect the problem is that nsFrameMessageManager::LoadFrameScriptInternal isn't reporting any kind of failure if JS_ExecuteScriptInternal returns false:

>631        JS_ExecuteScript(mCx, global,
>632                         (JSScript*)JS_GetPrivate(mCx, holder->mObject),
>633                         &val);

>706          JS_ExecuteScript(mCx, global,
>707                           (JSScript*)JS_GetPrivate(mCx, scriptObj), &val);

Comment 3

[Josh Matthews [:jdm]]

Hmm, I'm at least partially wrong.  I still suspect that we need to call JS_ReportPendingError if those JS_ExecuteScript calls fail, but a flat-out syntax error in a content script is causing JS_CompileUCScriptForPrincipals to return a null script object.  I don't see any built-in way that that gets reported (since we can end up loading lots and lots of frame scripts at weird points from one call to LoadFrameScript, returning non-NS_OK doesn't seem right), so I guess I need to manually report the error.

Comment 4

[Josh Matthews [:jdm]]

Good, my hunch is validated.  Calling an undefined function in a content script results in JS_ExecuteScript returning false, but strangely there doesn't seem to be an exception pending.

Comment 5

[Josh Matthews [:jdm]]

For those following along at home, I've now learned about JS_SetErrorReporter and how the message manager doesn't call it.  I'm going to throw together an error reporter and see what happens.

Comment 6

[Josh Matthews [:jdm]]

Attachment: Add error reporting for content scripts.

Comment 7

[Josh Matthews [:jdm]]

With this patch, mochitests still only show a generic "SpecialPowers is not defined".  However, if you look in the error console you'll see errors with links pointing to the culprit content script.  I'm not quite sure how the mochitest errors are reported but I suspect it's with window.onerror, which doesn't seem like the correct place for these errors to be showing up.

Comment 8

[(not currently active) Ted Mielczarek]

The error console would be absolutely perfect. That's where I was looking for errors (and not finding them). Plus, I think we echo that text to stdout or stderr in debug builds, right?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 520125 [details] [diff] [review]
Add error reporting for content scripts.

why the (void) in few places? Does some compiler give
warnings without those?

I wish we could reuse NS_ScriptErrorReporter, but that is way
too DOM-bound atm :(

Comment 10

[Josh Matthews [:jdm]]

I added the (void)s because I want to show that we are explicitly choosing to do nothing special when the scripts fail to execute.

Comment 11

[Josh Matthews [:jdm]]

Hmm, forgot about this.  Let's push it.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

patching file content/base/src/nsFrameMessageManager.cpp
Hunk #3 FAILED at 671
Hunk #4 FAILED at 746

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

The two hunks that fail only prefix the "(void)" return type handler to the existing code. The previous code was changed by:
http://hg.mozilla.org/mozilla-central/diff/300e998d0291/content/base/src/nsFrameMessageManager.cpp

So I just added the "(void)" prefix to the new code.

Testing

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mozilla-central/rev/fb1ebb23a1e2