Closed Bug 909764 Opened 6 years ago Closed 6 years ago

Assertion failure: parent, at jswrapper.cpp:35 or Assertion failure: !cx->isExceptionPending(), at jsapi.cpp:2902

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed

People

(Reporter: decoder, Assigned: djvj)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update,ignore])

Attachments

(6 files, 2 obsolete files)

The following testcase asserts on mozilla-central revision e42dce3209da (threadsafe build, run with --fuzzing-safe --ion-eager):


function testcase()  {
  "use strict";
  try {} catch(eval) {}
}
evaluate("\
var g1 = newGlobal('new-compartment');\
testcase.eval('function f() {}');\
");
I found this originally on mozilla-beta but it also reproduces on mozilla-central with a different assertion (and it doesn't seem to require a threadsafe build).

It would be very nice to get this fixed because this is one of the assertions that popup all the time, associated with OOM, but that are hard to get a test for.
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
Due to skipped revisions, the first bad revision could be any of:
changeset:   http://hg.mozilla.org/mozilla-central/rev/957a80f3ddab
user:        Bobby Holley
date:        Wed Jul 17 11:53:51 2013 -0700
summary:     Bug 887334 - Move faulty JSAutoRequest in initSelfHosting. r=luke

changeset:   http://hg.mozilla.org/mozilla-central/rev/31d4bde6b786
user:        Bobby Holley
date:        Wed Jul 17 11:53:52 2013 -0700
summary:     Bug 887334 - Add a JSCompartment* to AutoCompartment and use it in JS_NewGlobalObject. r=luke

This iteration took 344.416 seconds to run.
Needinfo from Bobby based on comment 3 :)
Flags: needinfo?(bobbyholley+bmo)
I just debugged this, along with djvj. My patches are a red herring. The problem is that jit::PropertyReadNeedsTypeBarrier is treated as a pure predicate, but it can actually throw. I'll attach a stack.

djvj is going to take over.
Assignee: general → kvijayan
Flags: needinfo?(bobbyholley+bmo)
Attached file culprit.txt
To expand on the issue, PropertyReadNeedsTypeBarrier calls  |TypeSet::getTypeOrSingleObject|.

getTypeOrSingleObject may call singleton->uninlinedGetType(cx), which will only return a null TypeObject if there's an error.  However, |getTypeOrSingleObject| is declared as returning a pointer to a single TypeObject, which may be null if there is more than one object-type in the typeset.

So, in |getTypeOrSingleObject|, returning NULL is not an error condition, but it has no other way of signalling error.
So I fixed this issue, in Ion, and how errors raised by getTypeOrSingleObject get propogated through the Ion codegen and out.

Now the same code throws an OOM exception.

However, this is a fake OOM exception.. in reality the exception is that the parsing of |testcase| throws an exception because it redefines eval in a strict function and that's deprecated.

It's the VM behaviour that's actually at fault here.  It should error out early because |testcase| has bad syntax, but it doesn't, probably because of lazy parsing.

For example, the following script:

function zoom() {
  "use strict";
  var eval = 9;
}
/* zoom is never called. */

Fails at parse time.  However, the following does not fail at parse time:

function zoom() {
  "use strict";
  try {} catch (eval) {}
}
/* zoom is never called. */

Changing the above slightly, we can cause it to throw the parse error by:

function zoom() {
  "use strict";
  try {} catch (eval) {}
}
zoom();

An additional error is that the lazy (checking) parser doesn't emit a parse error on |catch(eval)| pattern in strict mode, but the non-lazy (bytecode-generating parser) does.
Tentative issue:

SytnaxParserHandler::addCatchBlock is a trivial, empty method that returns true.

FullParseHandler::addCatchBlock is not trivial, and adds the parse nodes from the catch block for later handling.

Changing SyntaxParseHandler::addCatchBlock to check if parsing is in strict mode, and if so, ensure that the catchName node is not one of the non-redefinable special names.
Attached patch patch?Splinter Review
This is a solution I came up with while looking at this testcase with Kannan. Basically we check the PropertName before even desugaring and attaching the try block. This is similar to how we handle yield.
Attachment #798052 - Attachment is patch: true
Comment on attachment 798052 [details] [diff] [review]
patch?

Review of attachment 798052 [details] [diff] [review]:
-----------------------------------------------------------------

That patch would solve this immediate problem, I think.  But wouldn't it be better for this check to be in Parser<SyntaxParseHandler>::bindLet?  That would be consistent with where the error's thrown for |function f() { 'use strict'; let eval; }| (which, hilariously, hits a checkStrictBindings call in Parser<FullParseHandler>::bindVarOrConst).  It'd also be consistent with |function f() { 'use strict'; [0 for (eval in { x: 2 })]; }| which complains in Parser<FullParseHandler>::bindLet.  Basically, if the full-handler version checks, the syntax version should as well.

It seems to me we probably should do an audit of every checkStrictBindings call, and every other place that throws strict mode syntax errors, to be sure that the call occurs in both the full, and the parse, parametrizations of the parser.
There are two issues to fix (lazy parse error, and the fact that getTypeOrSingleObject is fallible, but its signature doesn't allow it to error), this patch fixes the first.

getTypeOrSingleObject's signature is changed to a fallible variant.  Its users are changed to use the new signature.  This makes some other infallible methods into fallible methods.  Their signatures are also changed.

Ultimately, IonBuilder is changed to use the new getTypeOrSingleObject signature, as well as new signature for other changed methods.

Also, a "false" result by IonBuilder::traverseBytecode would only ever generate an "can't compile" response from IonBuilder.  This prevented IonBuilder from propogating exceptions raised during build time.  IonBuilder has also thus been fixed to check the context for exceptions when traverseBytecode returns false, and throw an exception in that case.
Attachment #798974 - Flags: review?(hv1989)
Comment on attachment 798981 [details] [diff] [review]
Fix syntax parser error with code that binds |eval| in strict mode.

Review of attachment 798981 [details] [diff] [review]:
-----------------------------------------------------------------

I think I want a second parser-person to look at this and verify this is the rightest place for the fix.

::: js/src/frontend/Parser.cpp
@@ +2802,1 @@
>      return true;

Not sure if I'd just make this |return parser->checkStrictBinding(...)|, but this looks fine per my previous logic.
Attachment #798981 - Flags: review?(jwalden+bmo)
Attachment #798981 - Flags: review?(jorendorff)
Attachment #798981 - Flags: review+
Comment on attachment 798981 [details] [diff] [review]
Fix syntax parser error with code that binds |eval| in strict mode.

Review of attachment 798981 [details] [diff] [review]:
-----------------------------------------------------------------

Yep. Darn.
Attachment #798981 - Flags: review?(jorendorff) → review+
Can we get a handful of tests for this, not just catch-blocks, but also let-declarations, const-declarations, array-comprehensions, and generator-expressions?

(Kind of annoying we don't have an even slightly general way to apply existing parser tests to the lazy parser. Anyone see a quick hack we could do?)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #14)
> Not sure if I'd just make this |return parser->checkStrictBinding(...)|, but
> this looks fine per my previous logic.

I don't want to suggest that the nature of |::bindLet| is inherently reflected by |checkStrictBinding|.  Logically, bindLet does a bunch of checks and actions if none of them fail, returns without error.

I'd like to reflect that intent in the source code, by preserving the "list of shortcutting conditions, followed by a fallback return" pattern.

Syntax fix pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df11d75d3271
Added [leave open] (one more patch incoming), and  in-testsuite? to bug.

I have about half a dozen bugs on a list of bugs I want to write and push test cases for.  I'll add this bug to the list, and hopefully soon have a "push test cases only day" :)
Flags: in-testsuite?
Whiteboard: [jsbugmon:update] → [jsbugmon:update] [leave open]
(In reply to Kannan Vijayan [:djvj] from comment #18)
> I have about half a dozen bugs on a list of bugs I want to write and push
> test cases for.  I'll add this bug to the list, and hopefully soon have a
> "push test cases only day" :)

I dunno how everyone else on the team feels about it, but I really don't like this "hopefully".

Generally speaking, testable changes shouldn't go in without tests. There can be exceptions, but this bug isn't one of them. A test case was even provided with the bug report.

If I were the primary reviewer here, I would have r+'d conditionally on test coverage. I regret not making comments to that effect in my review anyway. It was a mistake. It didn't occur to me that a request for tests would be met with a response like "hopefully soon"!

In short, I don't think that response reflects the commitment to correctness we want to have as a team. Let's get this tested today or back it out. I'll help.
Yeah, I think I agree with comment 19.  (I plead insomnia last night as defense for not thinking to say anything before.)  At the very least, the reported testcase (and any discovered variations, whether failing or not -- basically the cleaned-up parts in comment 8) should be part of any patch posted for review (excepting security bugs, where the two should be posted separately, but at the same time).  Beyond that depends on patch complexity and all that.

I could probably live with just comment 8's tests, but that's partly a result of history where testing in Mozilla wasn't quite so second-nature, and just getting a test was a reasonable threshold for the time.  These days I probably should recalibrate to require more testing, as a rule, come to think of it.
Good points on Comment 19 and Comment 20.  I'll go ahead and move these test cases in before going ahead.
Attached patch testcase-bug-909764.patch (obsolete) — Splinter Review
Test case for catching strict-mode rebindings of |eval|.  There are about 7 simple cases being tested.  Without the syntax fix patch, 6/7 of them pass (with the try/catch testcase failing).  With the syntax fix patch, all pass.
Attachment #799075 - Flags: review?(jorendorff)
Comment on attachment 799075 [details] [diff] [review]
testcase-bug-909764.patch

Canceling review on this while I add a few more test cases.
Attachment #799075 - Flags: review?(jorendorff)
https://hg.mozilla.org/mozilla-central/rev/df11d75d3271
Whiteboard: [jsbugmon:update] [leave open] → [jsbugmon:update][leave open]
Whiteboard: [jsbugmon:update][leave open] → [leave open] [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision e3785e299ab6).
Comment on attachment 798974 [details] [diff] [review]
Fix getTypeOrSingleObject and IonBuilder error propogation.

Review of attachment 798974 [details] [diff] [review]:
-----------------------------------------------------------------

Since ionbuilder is actually not executing code. How can we get an exception? Shouldn't ionbuilder leave the state the same?
(In reply to Hannes Verschore [:h4writer] from comment #26)
> Comment on attachment 798974 [details] [diff] [review]
> Fix getTypeOrSingleObject and IonBuilder error propogation.
> 
> Review of attachment 798974 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since ionbuilder is actually not executing code. How can we get an
> exception? Shouldn't ionbuilder leave the state the same?

The exception happens in the parser.  IonBuilder can try to instantiate a lazy type for a script.  If the script hasn't been fully parsed yet, then this can induce a parse.  Parsing can OOM.
Updated testcases.
Attachment #799075 - Attachment is obsolete: true
Attachment #799566 - Flags: review?(jorendorff)
Attachment #799566 - Flags: review?(jorendorff)
Thanks, Kannan.
Attachment #796036 - Attachment is obsolete: true
Comment on attachment 798974 [details] [diff] [review]
Fix getTypeOrSingleObject and IonBuilder error propogation.

Review of attachment 798974 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/Ion.cpp
@@ +1652,5 @@
>      if (!builder->build()) {
> +        if (cx->isExceptionPending()) {
> +            IonSpew(IonSpew_Abort, "Builder raised exception.");
> +            return AbortReason_Error;
> +        }

Couple of questions:
1) This Error can this get catched in JS? In other words, can code still keep executing after we throw an exception during ionbuilder? I'm just wondering, since if we fail in buildInline we will disable inlining that function. So if we can still run further even when Error has been raised, than we could disable this optimization. Not sure if we want that. Should we also add some code to buildInline?

2) Can we have an exception pending when the builder didn't fail?

::: js/src/jit/IonBuilder.cpp
@@ +5561,5 @@
>      {
> +        return false;
> +    }
> +
> +    if (!shape || holder != templateObject || writeNeedsBarrier) {

This change makes me a bit nervous. I have a feeling that we shouldn't run PropertyWriteNeedsTypeBarrier if there is no shape or holder doesn't equal templateObject. And that it could fail badly, because that function assumes this was checked before...

@@ +6255,5 @@
>  
>      types::StackTypeSet *types = types::TypeScript::BytecodeTypes(script(), pc);
> +    bool barrier;
> +    if (!PropertyReadNeedsTypeBarrier(cx, staticType, name, types, /*updateObserved=*/true,
> +                                      &barrier))

Nit: add spaces
/* updateObserved = */ true

@@ +7201,5 @@
>  
>      if (!ElementAccessIsDenseNative(object, index))
>          return true;
> +    bool needsBarrier;
> +    if (!PropertyWriteNeedsTypeBarrier(cx, current, &object, NULL, &value, /*canModify=*/true,

Nit: Add spaces so it resembles this:
/* canModify = */ true

@@ +7267,5 @@
>      if (!icInspect.sawDenseWrite())
>          return true;
>  
> +    bool needsBarrier;
> +    if (PropertyWriteNeedsTypeBarrier(cx, current, &object, NULL, &value, /*canModify=*/true,

Nit: Add spaces so it resembles this:
/* canModify = */ true

@@ +8448,5 @@
>  
>      types::StackTypeSet *objTypes = obj->resultTypeSet();
> +    bool barrier;
> +    if (!PropertyWriteNeedsTypeBarrier(cx, current, &obj, name, &value,
> +                                       /*canModify=*/true, &barrier))

Nit: Add spaces so it resembles this:
/* canModify = */ true

::: js/src/jit/MCallOptimize.cpp
@@ +1011,5 @@
>          MDefinition *id = callInfo.getArg(idxi);
>          MDefinition *elem = callInfo.getArg(elemi);
>  
> +        bool writeNeedsBarrier;
> +        if (!PropertyWriteNeedsTypeBarrier(cx, current, &obj, NULL, &elem, /* canModify=*/ false,

Nit: Add spaces so it resembles this:
/* canModify = */ true

@@ +1021,4 @@
>          // We can only inline setelem on dense arrays that do not need type
>          // barriers and on typed arrays.
>          ScalarTypeRepresentation::Type arrayType;
> +        if ((!ElementAccessIsDenseNative(obj, id) || writeNeedsBarrier) &&

Can we keep the same order of running the conditionals?

::: js/src/jit/MIR.cpp
@@ +2544,5 @@
> +        types::TypeObject *object;
> +        if (!types->getTypeOrSingleObject(cx, i, &object))
> +            return false;
> +
> +        if (object) {

Nit: remove indentation by inverting:

if (!object)
   continue;

@@ +2569,5 @@
>  }
>  
>  bool
>  jit::PropertyReadNeedsTypeBarrier(JSContext *cx, types::TypeObject *object, PropertyName *name,
> +                                  types::StackTypeSet *observed, bool updateObserved, bool *result)

Why did you change this. I see no code that ever returns false? So this is infallible. So we don't need this change?

@@ +2582,3 @@
>  
>      if (object->unknownProperties())
>          return true;

I think we should do *result = true here to have the same behaviour as before.

@@ +2667,5 @@
> +
> +        if (object) {
> +            if (!PropertyReadNeedsTypeBarrier(cx, object, name, observed, updateObserved, result))
> +                return false;
> +            return true;

I think you miss here something to have the same meaning:

if (!PropertyReadNeedsTypeBarrier(cx, object, name, observed, updateObserved, result))
    return false;

if (*result)
    return true;
Attachment #798974 - Flags: review?(hv1989) → review+
(In reply to Hannes Verschore [:h4writer] from comment #33)
> 1) This Error can this get catched in JS? In other words, can code still
> keep executing after we throw an exception during ionbuilder? I'm just
> wondering, since if we fail in buildInline we will disable inlining that
> function. So if we can still run further even when Error has been raised,
> than we could disable this optimization. Not sure if we want that. Should we
> also add some code to buildInline?

I think we want to not disable inlining if we know for sure that the abort happened because of an error, and not some other issue.  I'll make that change.

> 2) Can we have an exception pending when the builder didn't fail?

No, this should never be the case.


> @@ +2569,5 @@
> >  }
> >  
> >  bool
> >  jit::PropertyReadNeedsTypeBarrier(JSContext *cx, types::TypeObject *object, PropertyName *name,
> > +                                  types::StackTypeSet *observed, bool updateObserved, bool *result)
> 
> Why did you change this. I see no code that ever returns false? So this is
> infallible. So we don't need this change?

I changed it to match the signature change in the other variant of PropertyReadNeedsTypeBarrier.  I think it's confusing to have two methods with the same name and similar duties, and one calls the other, but their signatures are subtly different - one returning its result, and the other returning its success-status and storing the result in a var.
This significantly (6+%) regressed the "dromaeo CSS" (aka jquery and similar libraries) performance tests across the board.  The jquery parts got 15+% slower.

I filed bug 913752 on this.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open] [jsbugmon:update,ignore] → [jsbugmon:update,ignore]
Is this something to consider uplifting to Aurora (Fx25)? Please set status-firefox25 to wontfix if not :)
Target Milestone: --- → mozilla26
I think it's OK not to consider this for uplift.  At most this will cause a swallowed exception in some rare cases, and the bug has existed for quite a while without manifesting in any other way.  I don't think it's serious enough to need uplift.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38)
> Please set status-firefox25 to wontfix if not :)

Allow me then.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #40)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38)
> > Please set status-firefox25 to wontfix if not :)
> 
> Allow me then.

Apologies, I thought I did that.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.