Errors in content scripts don't get displayed anywhere

RESOLVED FIXED

Status

()

Core
IPC
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: ted, Assigned: jdm)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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.)

Updated

7 years ago
Whiteboard: DUPEME
(Reporter)

Comment 1

7 years ago
I looked for a dupe, because I swore I filed this bug the last time I hit it, but I couldn't find one.
(Assignee)

Comment 2

7 years ago
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);
(Assignee)

Updated

7 years ago
Assignee: nobody → josh
(Assignee)

Comment 3

7 years ago
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.
(Assignee)

Comment 4

7 years ago
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.
(Assignee)

Comment 5

7 years ago
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.
(Assignee)

Comment 6

7 years ago
Created attachment 520125 [details] [diff] [review]
Add error reporting for content scripts.
Attachment #520125 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 7

7 years ago
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.
(Reporter)

Comment 8

7 years ago
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

7 years ago
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 :(
Attachment #520125 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 10

7 years ago
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.
(Assignee)

Comment 11

6 years ago
Hmm, forgot about this.  Let's push it.
Keywords: checkin-needed
patching file content/base/src/nsFrameMessageManager.cpp
Hunk #3 FAILED at 671
Hunk #4 FAILED at 746
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Reporter)

Updated

6 years ago
Whiteboard: DUPEME
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
pushed:
http://hg.mozilla.org/mozilla-central/rev/fb1ebb23a1e2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.