Last Comment Bug 642419 - Errors in content scripts don't get displayed anywhere
: Errors in content scripts don't get displayed anywhere
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Josh Matthews [:jdm] (on vacation until Dec 5)
:
: [PTO to Dec5] Bill McCloskey (:billm)
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description 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 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 Josh Matthews [:jdm] (on vacation until Dec 5) 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 Josh Matthews [:jdm] (on vacation until Dec 5) 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 Josh Matthews [:jdm] (on vacation until Dec 5) 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 Josh Matthews [:jdm] (on vacation until Dec 5) 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 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-03-17 21:09:16 PDT
Created attachment 520125 [details] [diff] [review]
Add error reporting for content scripts.
Comment 7 Josh Matthews [:jdm] (on vacation until Dec 5) 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 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 Olli Pettay [:smaug] 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 Josh Matthews [:jdm] (on vacation until Dec 5) 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 Josh Matthews [:jdm] (on vacation until Dec 5) 2011-04-12 16:57:33 PDT
Hmm, forgot about this.  Let's push it.
Comment 12 :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 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:
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?) 2011-05-13 06:12:05 PDT
pushed:
http://hg.mozilla.org/mozilla-central/rev/fb1ebb23a1e2

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