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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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() {}');\
");

Comment 1

[Christian Holler (:decoder)]

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.

Comment 2

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 3

[Christian Holler (:decoder)]

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.

Comment 4

[Christian Holler (:decoder)]

Needinfo from Bobby based on comment 3 :)

Comment 5

[Bobby Holley (:bholley)]

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.

Comment 6

[Bobby Holley (:bholley)]

Attachment: culprit.txt

Comment 7

[Kannan Vijayan [:djvj]]

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.

Comment 8

[Kannan Vijayan [:djvj]]

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.

Comment 9

[Kannan Vijayan [:djvj]]

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.

Comment 10

[Tom S [:evilpie]]

Attachment: patch?

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.

Comment 11

[Jeff Walden [:Waldo]]

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.

Comment 12

[Kannan Vijayan [:djvj]]

Attachment: Fix getTypeOrSingleObject and IonBuilder error propogation.

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.

Comment 13

[Kannan Vijayan [:djvj]]

Attachment: Fix syntax parser error with code that binds |eval| in strict mode.

Small fix.

Comment 14

[Jeff Walden [:Waldo]]

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.

Comment 15

[Jason Orendorff [:jorendorff]]

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.

Comment 16

[Jason Orendorff [:jorendorff]]

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?)

Comment 17

[Kannan Vijayan [:djvj]]

(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

Comment 18

[Kannan Vijayan [:djvj]]

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" :)

Comment 19

[Jason Orendorff [:jorendorff]]

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

Comment 20

[Jeff Walden [:Waldo]]

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.

Comment 21

[Kannan Vijayan [:djvj]]

Good points on Comment 19 and Comment 20.  I'll go ahead and move these test cases in before going ahead.

Comment 22

[Kannan Vijayan [:djvj]]

Attachment: testcase-bug-909764.patch

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.

Comment 23

[Kannan Vijayan [:djvj]]

Comment on attachment 799075 [details] [diff] [review]
testcase-bug-909764.patch

Canceling review on this while I add a few more test cases.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df11d75d3271

Comment 25

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision e3785e299ab6).

Comment 26

[Hannes Verschore [:h4writer]]

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?

Comment 27

[Kannan Vijayan [:djvj]]

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

Comment 28

[Kannan Vijayan [:djvj]]

Attachment: testcase-bug-909764.patch

Updated testcases.

Comment 29

[Kannan Vijayan [:djvj]]

Test case for syntax patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/2eec82741eb8

Comment 30

[Jason Orendorff [:jorendorff]]

Thanks, Kannan.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2eec82741eb8

Comment 32

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 33

[Hannes Verschore [:h4writer]]

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;

Comment 34

[Kannan Vijayan [:djvj]]

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

Comment 35

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1699c4e7afdc

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1699c4e7afdc

Comment 37

[Boris Zbarsky [:bzbarsky]]

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.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Is this something to consider uplifting to Aurora (Fx25)? Please set status-firefox25 to wontfix if not :)

Comment 39

[Kannan Vijayan [:djvj]]

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.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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

Allow me then.

Comment 41

[Kannan Vijayan [:djvj]]

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