700202 - "Assertion failure: !cx->isExceptionPending()" with nearNativeStackLimit, document.write

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

###!!! ASSERTION: This is not supposed to fail!: 'Error', file /Users/jruderman/trees/mozilla-central/js/xpconnect/src/nsXPConnect.cpp, line 943

Assertion failure: !cx->isExceptionPending(), at js/src/jscntxtinlines.h:300

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jesse Ruderman]

Attachment: JS_CHECK_RECURSION reporting patch

I used this while trying to make a reduced testcase. Might be useful for debugging, too.

Comment 3

[Steve Fink [:sfink] [:s:]]

I think this one is over my head.

It's hitting the recursion limit and throwing an over-recursed exception. The catch {} suppresses that exception. What happens next varies a bit from run to run. Sometimes when finishing up the execution of JSOP_EXCEPTION, it hits the recursion limit again while attempting to wrap a value for a cross-compartment call. Sometimes, it makes it past that and hits the recursion limit in something else, for a variety of something elses.

But here's at least one that definitely seems wrong to me:

XPCConvert::NativeInterface2JSObject hits its tryConstructSlimWrapper case and calls ConstructSlimWrapper(). It fails because of the recursion limit, and returns false. It continues on, commenting that the wrapper may have been constructed anyway, but flat == NULL so it wasn't.

Knowing that it doesn't have a slim wrapper, it proceeds to create an XPCWrappedNative, which succeeds. It then continues along happily, putting the earlier failure out of its mind -- except that the context still has a pending exception set, and it grows in strength and bitterness until it returns in force to overthrow the kingdom.

So a potential fix is to clear the pending exception if the slim wrapper creation fails, but I don't even know what these words I'm typing mean.

Also, after having done that, it still fails. Now the problem seems to be in nsJSEnvironment.cpp:

  // If we get here we're most likely executing an infinite loop in JS,
  // we'll tell the user about this and we'll give the user the option
  // of stopping the execution of the script.
  nsCOMPtr<nsIPrompt> prompt = GetPromptFromContext(ctx);
  NS_ENSURE_TRUE(prompt, JS_TRUE);
   ...proceed to use prompt to pop up a dialog...

Specifically, I don't understand the NS_ENSURE_TRUE(prompt, JS_TRUE) part. In our case, GetPromptFromContext failed and returned a null prompt. So now we're supposed to lie and say that everything was great? But that looks intentional, and changing it doesn't help. (Or maybe it does, but it still doesn't fix things.)

...and now nsIDOMHTMLDocument_Write is returning NS_OK after having set an exception. Stupid princess.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: NativeInterface2JSObject error handling

Comment 5

[Bobby Holley (:bholley)]

If ConstructSlimWrapper throws proper (by setting an exception on cx), I think we should throw too.

Really, there are two failure cases being communicated here. The first is a standard JS exception. The second is "I tried and was unable to construct a slim wrapper, but everything else is fine". 

It's clear that we don't want to throw for the second here, but I think we should for the first, rather than just clearing the pending exception.

Presumably, the second failure case doesn't set a pending exception. However, I don't know enough to enumerate (with certainty) the cases where ConstructSlimWrapper fails but we still want to continue.

CCing peterv, because hopefully he can.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: error check ReparentContentWrappersInScope

With this additional fix, the test case passes for me.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 575602 [details] [diff] [review]
error check ReparentContentWrappersInScope

>+    NS_ASSERTION(!JS_IsExceptionPending(cx), "succeeded with exception set");

Can you use NS_ABORT_IF_FALSE to avoid adding any new non-fatal assertions?

Comment 8

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

Comment on attachment 575602 [details] [diff] [review]
error check ReparentContentWrappersInScope

Ok, I think we should do this, but I'm not sure if this is just
hiding the actual problem.

Jesse, could you try a testcase which has try-catch around document.write.

Comment 9

[Jesse Ruderman]

There's already a try-catch around it, in the way nearNativeStackLimit calls f.

Comment 10

[Steve Fink [:sfink] [:s:]]

(In reply to Bobby Holley (:bholley) from comment #5)
> If ConstructSlimWrapper throws proper (by setting an exception on cx), I
> think we should throw too.

What do you mean by "throw"? Do you just mean return JS_FALSE?

> Really, there are two failure cases being communicated here. The first is a
> standard JS exception. The second is "I tried and was unable to construct a
> slim wrapper, but everything else is fine". 
> 
> It's clear that we don't want to throw for the second here, but I think we
> should for the first, rather than just clearing the pending exception.
> 
> Presumably, the second failure case doesn't set a pending exception.
> However, I don't know enough to enumerate (with certainty) the cases where
> ConstructSlimWrapper fails but we still want to continue.

Currently, it will attempt to continue after *all* failures, so at least adding

  if (JS_IsExceptionPending(cx))
    return JS_FALSE;

would be safe. There might be other ways to fail after which we shouldn't bother to continue, and it wouldn't improve any of those (hypothetical) cases, but at least it would do no harm.

(In reply to Bobby Holley (:bholley) from comment #7)
> Comment on attachment 575602 [details] [diff] [review] [diff] [details] [review]
> error check ReparentContentWrappersInScope
> 
> >+    NS_ASSERTION(!JS_IsExceptionPending(cx), "succeeded with exception set");
> 
> Can you use NS_ABORT_IF_FALSE to avoid adding any new non-fatal assertions?

Sure. I can never keep those macros straight in my head.

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 575602 [details] [diff] [review] [diff] [details] [review]
> error check ReparentContentWrappersInScope
> 
> Ok, I think we should do this, but I'm not sure if this is just
> hiding the actual problem.

The actual problem in this case is stack overflow, and the patch is *exposing* it, so I'm not sure what you mean by "hiding the actual problem".

Comment 11

[Bobby Holley (:bholley)]

(In reply to Steve Fink [:sfink] from comment #10)

> What do you mean by "throw"? Do you just mean return JS_FALSE?

Yes - allowing the exception to propagate rather than squelching it.
> Currently, it will attempt to continue after *all* failures, so at least
> adding
> 
>   if (JS_IsExceptionPending(cx))
>     return JS_FALSE;
> 
> would be safe. There might be other ways to fail after which we shouldn't
> bother to continue, and it wouldn't improve any of those (hypothetical)
> cases, but at least it would do no harm.

Yeah - if we can't track down mrbkap or peterv this would be my preferred approach.

Comment 12

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

https://hg.mozilla.org/try/rev/cbc486c6eca7

looks fishy to me... Wouldn't a boolean return value to indicate success/failure (as typical in js), and a boolean out-param to indicate whether a wrapper was created work as well? That seems more obvious than checking some state on cx that the function you call might have changed.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Ms2ger from comment #12)

> looks fishy to me... Wouldn't a boolean return value to indicate
> success/failure (as typical in js), and a boolean out-param to indicate
> whether a wrapper was created work as well? That seems more obvious than
> checking some state on cx that the function you call might have changed.

I agree that the situation isn't ideal. In a perfect world, I'd rename ConstructSlimWrapper to TryConstructingSlimWrapper, and indicate successful construction with the value stored in &slim. But that requires a careful audit of ConstructSlimWrapper, and we'll be ripping slim wrappers out in a few months anyway.

Comment 14

[Steve Fink [:sfink] [:s:]]

Attachment: NativeInterface2JSObject error handling

I'll put this up for review, and land it soon-ish unless peterv/mrbkap chime in with an objection.

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/4fe03ef7a1d8
https://hg.mozilla.org/mozilla-central/rev/2cddf1a5d9e2