Last Comment Bug 700202 - "Assertion failure: !cx->isExceptionPending()" with nearNativeStackLimit, document.write
: "Assertion failure: !cx->isExceptionPending()" with nearNativeStackLimit, doc...
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla11
Assigned To: general
:
Mentors:
Depends on: 707749
Blocks: 594645
  Show dependency treegraph
 
Reported: 2011-11-06 18:39 PST by Jesse Ruderman
Modified: 2011-12-05 10:29 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (asserts fatally when loaded) (847 bytes, text/html)
2011-11-06 18:39 PST, Jesse Ruderman
no flags Details
stack trace (4.73 KB, text/plain)
2011-11-06 18:39 PST, Jesse Ruderman
no flags Details
JS_CHECK_RECURSION reporting patch (1.86 KB, patch)
2011-11-06 18:47 PST, Jesse Ruderman
no flags Details | Diff | Splinter Review
NativeInterface2JSObject error handling (1.07 KB, patch)
2011-11-18 15:17 PST, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Splinter Review
error check ReparentContentWrappersInScope (1.69 KB, patch)
2011-11-18 16:26 PST, Steve Fink [:sfink] [:s:]
bugs: review+
Details | Diff | Splinter Review
NativeInterface2JSObject error handling (1.12 KB, patch)
2011-11-22 15:30 PST, Steve Fink [:sfink] [:s:]
bobbyholley: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-11-06 18:39:14 PST
Created attachment 572372 [details]
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 2011-11-06 18:39:46 PST
Created attachment 572373 [details]
stack trace
Comment 2 Jesse Ruderman 2011-11-06 18:47:21 PST
Created attachment 572375 [details] [diff] [review]
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:] 2011-11-18 15:12:00 PST
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:] 2011-11-18 15:17:19 PST
Created attachment 575584 [details] [diff] [review]
NativeInterface2JSObject error handling
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2011-11-18 15:44:05 PST
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:] 2011-11-18 16:26:48 PST
Created attachment 575602 [details] [diff] [review]
error check ReparentContentWrappersInScope

With this additional fix, the test case passes for me.
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2011-11-18 16:43:01 PST
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] (vacation Aug 25-28) 2011-11-19 11:02:40 PST
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 2011-11-19 13:01:26 PST
There's already a try-catch around it, in the way nearNativeStackLimit calls f.
Comment 10 Steve Fink [:sfink] [:s:] 2011-11-20 22:26:31 PST
(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) (busy with Stylo) 2011-11-20 22:31:34 PST
(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 (⌚ UTC+1/+2) 2011-11-22 03:57:29 PST
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) (busy with Stylo) 2011-11-22 09:11:46 PST
(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:] 2011-11-22 15:30:08 PST
Created attachment 576323 [details] [diff] [review]
NativeInterface2JSObject error handling

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

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