704259 - try/catch blocks seem to be ignored in window.onerror when the first error originated from addEventListener, but not from onclick

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason J. Rotello]

Attachment: 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]

bug 503244?

Comment 2

[Nickolay_Ponomarev]

Attachment: 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]

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]

Attachment: testcase

Thanks! Attached a testcase not using jQuery at all, just to be sure.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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]]

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]]

Attachment: Part 1: refactor generatingError uses

This doesn't fix anything, it just gets rid of a bunch of gotos.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 603548 [details] [diff] [review]
Part 1: refactor generatingError uses

s/JS_FALSE/false/

Comment 9

[David Mandelin [:dmandelin]]

Attachment: 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 [:bzbarsky]]

> so I consider it lost to the ages.

Check cvsblame?  ;)

Comment 11

[Luke Wagner [:luke]]

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 12

[David Mandelin [:dmandelin]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3b81add7efd2
http://hg.mozilla.org/integration/mozilla-inbound/rev/578789b11b81

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/3b81add7efd2
https://hg.mozilla.org/mozilla-central/rev/578789b11b81

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: 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][[e-mail]]]

Please back out the patches.

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/8219e6519190

Comment 17

[Bill Gianopoulos [:WG9s]]

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]]

(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]]

Attachment: Part 1 (refactoring), v2

Comment 20

[David Mandelin [:dmandelin]]

The new patch seems to work, and doesn't hit bug 734167.

Comment 21

[Luke Wagner [:luke]]

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]]

(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]]

Attachment: Part 1 (refactoring), v3

Comment 24

[Luke Wagner [:luke]]

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 25

[David Mandelin [:dmandelin]]

Relanded:

http://hg.mozilla.org/integration/mozilla-inbound/rev/c5a948233453
http://hg.mozilla.org/integration/mozilla-inbound/rev/d32b8027cb46

Comment 26

[David Mandelin [:dmandelin]]

(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]]

(Missed the uplift, changing target milestone to 14).

Comment 28

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

We've got AutoRestore in xpcom/glue/AutoRestore.h, maybe move that to MFBT?

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c5a948233453
https://hg.mozilla.org/mozilla-central/rev/d32b8027cb46

Comment 30

[Alex Keybl [:akeybl]]

Dave - you tracked this bug for FF13. Should we consider an uplift to Aurora?

Comment 31

[David Mandelin [:dmandelin]]

(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]]

(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.