Last Comment Bug 704259 - try/catch blocks seem to be ignored in window.onerror when the first error originated from addEventListener, but not from onclick
: try/catch blocks seem to be ignored in window.onerror when the first error or...
Status: RESOLVED FIXED
: reproducible, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: David Mandelin [:dmandelin]
:
Mentors:
Depends on: 734167
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-21 13:25 PST by Jason J. Rotello
Modified: 2012-04-10 15:29 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
-


Attachments
sample.html (1.09 KB, text/plain)
2011-11-21 13:25 PST, Jason J. Rotello
no flags Details
testcase demonstrating different behavior with and without jQuery (1.55 KB, text/html)
2011-11-26 13:23 PST, Nickolay_Ponomarev
no flags Details
testcase (1.49 KB, text/html)
2011-11-27 23:46 PST, Nickolay_Ponomarev
no flags Details
Part 1: refactor generatingError uses (4.45 KB, patch)
2012-03-06 18:23 PST, David Mandelin [:dmandelin]
luke: review+
Details | Diff | Splinter Review
Part 2: candidate fix (1.15 KB, patch)
2012-03-06 18:27 PST, David Mandelin [:dmandelin]
luke: review+
Details | Diff | Splinter Review
infinite loop crash stack trace (23.91 KB, text/plain)
2012-03-07 18:00 PST, Nicholas Nethercote [:njn]
no flags Details
Part 1 (refactoring), v2 (5.44 KB, patch)
2012-03-09 18:33 PST, David Mandelin [:dmandelin]
no flags Details | Diff | Splinter Review
Part 1 (refactoring), v3 (6.06 KB, patch)
2012-03-12 18:04 PDT, David Mandelin [:dmandelin]
luke: review+
Details | Diff | Splinter Review

Description Jason J. Rotello 2011-11-21 13:25:17 PST
Created attachment 575953 [details]
sample.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.2 (KHTML, like Gecko) Chrome/15.0.874.121 Safari/535.2

Steps to reproduce:

Raising an exception in the window.onerror event handler seems to ignore try/catch blocks and stops execution. I would expect a try/catch block in the window.onerror hander to be treated the same as any other location where it is used. 

Please see the following StackOverflow question related to this as well. http://bit.ly/uU5aoe


Actual results:

When clicking on the "Test" button in the attached sample file the following is output on the console window:

begin onerror


Expected results:

I would have expected the following output when clicking on the "Test" button in the attached sample:

begin onerror
catch block
end onerror

My expectation is based upon the results I see in IE, Chrome, and Safari.
Comment 1 Alice0775 White 2011-11-21 17:40:17 PST
bug 503244?
Comment 2 Nickolay_Ponomarev 2011-11-26 13:23:54 PST
Created attachment 577102 [details]
testcase demonstrating different behavior with and without jQuery

This is related to jQuery somehow. I bet replacing the jQuery dependency with the minimal code necessary to trigger this behavior will either explain this or make this easier to debug and fix on the Firefox side.
Comment 3 Dave Methvin 2011-11-27 18:04:02 PST
Seems to be related to addEventListener, doesn't happen with .onclick for example. Here is a test case: http://jsfiddle.net/dmethvin/2gpWB/

Related jQuery ticket: http://bugs.jquery.com/ticket/10904
Comment 4 Nickolay_Ponomarev 2011-11-27 23:46:24 PST
Created attachment 577201 [details]
testcase

Thanks! Attached a testcase not using jQuery at all, just to be sure.
Comment 5 Boris Zbarsky [:bz] 2011-12-02 15:10:12 PST
I think this is fundamentally a JS engine bug...

The working case has code flow as follows:

1) Click handler called
2) js_ErrorToException is entered
3) js_ErrorToException is exited
4) error reporter is called via AutoLastFrameCheck::~AutoLastFrameCheck calling
   js_ReportUncaughtException

The non-working case has code flow as follows:

1) Click handler called
2) js_ErrorToException is entered
3) js_ErrorToException is exited
4) error reporter is called via nsXPCWrappedJSClass::CallMethod calling
   nsXPCWrappedJSClass::CheckForException which calls JS_ReportPendingException which
   calls js_ReportUncaughtException.

The difference is that JS_ReportPendingException sets cx->generatingError to true before calling js_ReportUncaughtException, while ~AutoLastFrameCheck does not do that.

So in the second case we call the error handler, it tries to throw, js_ErrorToException is entered and says "oh, we're already in the middle of generating an error; just do the uncatchable thing".

The nice comment in JS_ReportPendingException is:

6142         * Set cx->generatingError to suppress the standard error-to-exception
6143         * conversion done by all {js,JS}_Report* functions except for OOM.  The
6144         * cx->generatingError flag was added to suppress recursive divergence
6145         * under js_ErrorToException, but it serves for our purposes here too.

I don't think it in fact serves for our purposes....
Comment 6 David Mandelin [:dmandelin] 2012-03-06 15:39:31 PST
This is not a regression, so not tracking for 12. But I'm taking it.

Boris, thanks for the analysis. The code in JS_ReportPendingException is pretty old, so it's hard to tell what the purpose was. I think I'm going to try removing that and see what happens. If that doesn't work, then we can probably just add a new API (or new argument to JS_RPE) for use by CheckForException.
Comment 7 David Mandelin [:dmandelin] 2012-03-06 18:23:24 PST
Created attachment 603548 [details] [diff] [review]
Part 1: refactor generatingError uses

This doesn't fix anything, it just gets rid of a bunch of gotos.
Comment 8 Luke Wagner [:luke] 2012-03-06 18:24:54 PST
Comment on attachment 603548 [details] [diff] [review]
Part 1: refactor generatingError uses

s/JS_FALSE/false/
Comment 9 David Mandelin [:dmandelin] 2012-03-06 18:27:54 PST
Created attachment 603552 [details] [diff] [review]
Part 2: candidate fix

The comment in the original code says that setting generatingError serves the programmer's intent, but doesn't deign to say what that intent is, and it's pre-hg code, so I consider it lost to the ages.

This patch fixes the problem and seems to be doing well on try, so I figure it's worth a shot.
Comment 10 Boris Zbarsky [:bz] 2012-03-06 18:35:01 PST
> so I consider it lost to the ages.

Check cvsblame?  ;)
Comment 11 Luke Wagner [:luke] 2012-03-06 18:44:43 PST
Comment on attachment 603552 [details] [diff] [review]
Part 2: candidate fix

Yeah, setting generatingError seems strange and unnecessary.  If try likes it, so do I.
Comment 14 Nicholas Nethercote [:njn] 2012-03-07 18:00:22 PST
Created attachment 603936 [details]
infinite loop crash stack trace

The first patch in this bug causes a reliable infinite loop crash for me when I start Chatzilla (I bisected it).  I'll attach the first chunk of the stack trace.
Comment 15 Olli Pettay [:smaug] 2012-03-08 06:23:34 PST
Please back out the patches.
Comment 16 Olli Pettay [:smaug] 2012-03-08 06:50:23 PST
https://hg.mozilla.org/mozilla-central/rev/8219e6519190
Comment 17 Bill Gianopoulos [:WG9s] 2012-03-08 07:26:53 PST
Comment on attachment 603548 [details] [diff] [review]
Part 1: refactor generatingError uses

>-    /*
>-     * Prevent runaway recursion, via cx->generatingError.  If an out-of-memory
>-     * error occurs, no exception object will be created, but we don't assume
>-     * that OOM is the only kind of error that subroutines of this function
>-     * called below might raise.
>-     */
>+    /* Prevent infinite recursion. */
>     if (cx->generatingError)
>         return JS_FALSE;
>-
>-    MUST_FLOW_THROUGH("out");
>-    cx->generatingError = JS_TRUE;
>+    AutoScopedAssign<bool> asa(&cx->generatingError, false);

Shouldn't this be setting GeneratingError to true like the old code did?  I see nothing in this code now that ever sets it to true.
Comment 18 David Mandelin [:dmandelin] 2012-03-09 18:31:22 PST
(In reply to Bill Gianopoulos [:WG9s] from comment #17)
> Comment on attachment 603548 [details] [diff] [review]
> Part 1: refactor generatingError uses
> 
> >-    /*
> >-     * Prevent runaway recursion, via cx->generatingError.  If an out-of-memory
> >-     * error occurs, no exception object will be created, but we don't assume
> >-     * that OOM is the only kind of error that subroutines of this function
> >-     * called below might raise.
> >-     */
> >+    /* Prevent infinite recursion. */
> >     if (cx->generatingError)
> >         return JS_FALSE;
> >-
> >-    MUST_FLOW_THROUGH("out");
> >-    cx->generatingError = JS_TRUE;
> >+    AutoScopedAssign<bool> asa(&cx->generatingError, false);
> 
> Shouldn't this be setting GeneratingError to true like the old code did?  I
> see nothing in this code now that ever sets it to true.

I think you spotted it. The last line should be 

    AutoScopedAssign<bool> asa(&cx->generatingError, true);
Comment 19 David Mandelin [:dmandelin] 2012-03-09 18:33:18 PST
Created attachment 604587 [details] [diff] [review]
Part 1 (refactoring), v2
Comment 20 David Mandelin [:dmandelin] 2012-03-09 19:07:59 PST
The new patch seems to work, and doesn't hit bug 734167.
Comment 21 Luke Wagner [:luke] 2012-03-10 12:31:02 PST
Comment on attachment 604587 [details] [diff] [review]
Part 1 (refactoring), v2

>-    MUST_FLOW_THROUGH("out");
>-    cx->generatingError = JS_TRUE;
>+        return false;
>+    AutoScopedAssign<bool> asa(&cx->generatingError, true);

It seems that the desired effect is for generatingError to be true for the remainder of the body of the function, and reset to false on return.  In that case, I think you want:

cx->generatingError = true;
AutoScopedAssign<bool> asa(&cx->generatingError, false);

>     ok = js_GetClassPrototype(cx, NULL, GetExceptionProtoKey(exn), &errProto);
>     if (!ok)
>-        goto out;

Here and a few places below you can merge the two lines (if (!js_GetClassPrototype...)) and perhaps remove 'ok' altogether.
Comment 22 David Mandelin [:dmandelin] 2012-03-12 18:03:02 PDT
(In reply to Luke Wagner [:luke] from comment #21)
> Comment on attachment 604587 [details] [diff] [review]
> Part 1 (refactoring), v2
> 
> >-    MUST_FLOW_THROUGH("out");
> >-    cx->generatingError = JS_TRUE;
> >+        return false;
> >+    AutoScopedAssign<bool> asa(&cx->generatingError, true);
> 
> It seems that the desired effect is for generatingError to be true for the
> remainder of the body of the function, and reset to false on return.  In
> that case, I think you want:
> 
> cx->generatingError = true;
> AutoScopedAssign<bool> asa(&cx->generatingError, false);

I'm pretty sure it's right the way it is: AutoScopedAssign stores argument 2 in the ctor, and stores 'old' in the dtor.
> 
> >     ok = js_GetClassPrototype(cx, NULL, GetExceptionProtoKey(exn), &errProto);
> >     if (!ok)
> >-        goto out;
> 
> Here and a few places below you can merge the two lines (if
> (!js_GetClassPrototype...)) and perhaps remove 'ok' altogether.

I did that. Revised patch coming momentarily.
Comment 23 David Mandelin [:dmandelin] 2012-03-12 18:04:54 PDT
Created attachment 605257 [details] [diff] [review]
Part 1 (refactoring), v3
Comment 24 Luke Wagner [:luke] 2012-03-12 21:33:47 PDT
Comment on attachment 605257 [details] [diff] [review]
Part 1 (refactoring), v3

Oops, you're right (about the patch and about what I mistakenly thought AutoScopedAssign did).  AutoScopedAssign is a confusing name (at least to me, clearly :).  Perhaps ScopedAssignAndRestore would be better (Auto seems redundant with Scoped).  There are currently 0 uses of AutoScopedAssign; so if you agree with that name or a better one, feel free to fix.
Comment 26 David Mandelin [:dmandelin] 2012-03-13 10:57:07 PDT
(In reply to Luke Wagner [:luke] from comment #24)
> AutoScopedAssign is a confusing name (at least to
> me, clearly :).  Perhaps ScopedAssignAndRestore would be better (Auto seems
> redundant with Scoped).  There are currently 0 uses of AutoScopedAssign; so
> if you agree with that name or a better one, feel free to fix.

I'm not sure about the naming issue. The name is kind of opaque to me as well, and I agree that Auto and Scoped are redundant. I'll see if I can think of a name I like today. I think the class deserves a brief usage comment too.
Comment 27 Ed Morley [:emorley] 2012-03-13 17:31:37 PDT
(Missed the uplift, changing target milestone to 14).
Comment 28 :Ms2ger (⌚ UTC+1/+2) 2012-03-14 08:29:14 PDT
We've got AutoRestore in xpcom/glue/AutoRestore.h, maybe move that to MFBT?
Comment 30 Alex Keybl [:akeybl] 2012-04-09 15:51:18 PDT
Dave - you tracked this bug for FF13. Should we consider an uplift to Aurora?
Comment 31 David Mandelin [:dmandelin] 2012-04-09 15:55:52 PDT
(In reply to Alex Keybl [:akeybl] from comment #30)
> Dave - you tracked this bug for FF13. Should we consider an uplift to Aurora?

I think I recommend against:

 - It's not a regression, we've had it at least since 3.
 - We're not aware of web sites in the field being broken by it.
 - I don't actually understand the code that I patched for the fix, so the risks are hard to analyze.

I tracked it because it does seem like a significant behavioral bug and I wanted to fix it promptly.
Comment 32 Alex Keybl [:akeybl] 2012-04-10 15:29:40 PDT
(In reply to David Mandelin from comment #31)
> I tracked it because it does seem like a significant behavioral bug and I
> wanted to fix it promptly.

Thanks Dave. We'll untrack for FF13 in that case.

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