Last Comment Bug 642419 - Errors in content scripts don't get displayed anywhere
: Errors in content scripts don't get displayed anywhere
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To: Josh Matthews [:jdm]
: Bill McCloskey (:billm)
Depends on:
  Show dependency treegraph
Reported: 2011-03-17 05:26 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2011-05-13 06:12 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Add error reporting for content scripts. (7.73 KB, patch)
2011-03-17 21:09 PDT, Josh Matthews [:jdm]
bugs: review+
Details | Diff | Splinter Review

Description User image Ted Mielczarek [:ted.mielczarek] 2011-03-17 05:26:57 PDT
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 User image Ted Mielczarek [:ted.mielczarek] 2011-03-17 06:38:06 PDT
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 User image Josh Matthews [:jdm] 2011-03-17 08:37:55 PDT
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 User image Josh Matthews [:jdm] 2011-03-17 12:16:42 PDT
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 User image Josh Matthews [:jdm] 2011-03-17 12:47:22 PDT
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 User image Josh Matthews [:jdm] 2011-03-17 16:45:32 PDT
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 User image Josh Matthews [:jdm] 2011-03-17 21:09:16 PDT
Created attachment 520125 [details] [diff] [review]
Add error reporting for content scripts.
Comment 7 User image Josh Matthews [:jdm] 2011-03-17 21:12:04 PDT
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 User image Ted Mielczarek [:ted.mielczarek] 2011-03-18 04:57:40 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-03-24 09:13:28 PDT
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 User image Josh Matthews [:jdm] 2011-03-24 09:20:07 PDT
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 User image Josh Matthews [:jdm] 2011-04-12 16:57:33 PDT
Hmm, forgot about this.  Let's push it.
Comment 12 User image :Ms2ger (⌚ UTC+1/+2) 2011-04-13 12:50:39 PDT
patching file content/base/src/nsFrameMessageManager.cpp
Hunk #3 FAILED at 671
Hunk #4 FAILED at 746
Comment 13 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-05-13 06:04:02 PDT
The two hunks that fail only prefix the "(void)" return type handler to the existing code. The previous code was changed by:

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

Comment 14 User image Mark Finkle (:mfinkle) (use needinfo?) 2011-05-13 06:12:05 PDT

Note You need to log in before you can comment on or make changes to this bug.