961494 - Assertion failure: obj->inDictionaryMode() || parent->hasMissingSlot() || child.slot() == parent->maybeSlot() + 1, at vm/Shape.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

let x;
a = new Error;
a["__count__"] = '';
Object.defineProperty(a, "__count__", ({
    configurable: x
}))

asserts js debug shell on m-c changeset 81bced59e8b3 without any CLI arguments at Assertion failure: obj->inDictionaryMode() || parent->hasMissingSlot() || child.slot() == parent->maybeSlot() + 1, at vm/Shape.cpp

My configure flags are:

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: regression window for threadsafe builds

Bug 724768 seems to involve the Error object and is in the regression window, setting needinfo? from Waldo.

Comment 2

[Jeff Walden [:Waldo]]

Shorter testcase:

var err = new Error;
err.expando = 17;
Object.defineProperty(err, "expando", { configurable: false });

Debugging does suggest this is triggered by bug 724768 -- investigation in progress.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Debugging does suggest this is triggered by bug 724768 -- investigation in
> progress.

Assigning since Waldo is on the case.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Don't assert that a property in a non-reserved slot immediately follows a last property in a non-reserved slot

I've said before that with respect to snippets of code, no comments often indicates bad code, some comments may indicate reasonable code (if perhaps tricky code), and long comments often indicate smelly code.  This is getting smelly.  :-\  But for an assertion, maybe it's not quite horrible.  Bleh.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8371824 [details] [diff] [review]
Don't assert that a property in a non-reserved slot immediately follows a last property in a non-reserved slot

Hmm, maybe let's add another eye or two here.  As I said, fugly.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8371824 [details] [diff] [review]
Don't assert that a property in a non-reserved slot immediately follows a last property in a non-reserved slot

Fuzzers: engage!

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8371824 [details] [diff] [review]
Don't assert that a property in a non-reserved slot immediately follows a last property in a non-reserved slot

Nothing found after an overnight run.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8371824 [details] [diff] [review]
Don't assert that a property in a non-reserved slot immediately follows a last property in a non-reserved slot

Oops.

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 8371824 [details] [diff] [review]
Don't assert that a property in a non-reserved slot immediately follows a last property in a non-reserved slot

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

::: js/src/vm/Shape.cpp
@@ +364,5 @@
> +             * predefined properties.  And of these, only Error objects can
> +             * have any trailing reserved slots not corresponding to a property
> +             * after creation -- for cases where Error objects don't have an
> +             * associated message.)
> +             */

I think this comment is obfuscatory more than anything else.  The comment needs to indicate why we're asserting what we're asserting, rather than spelling out the corner case found by this fuzzbug.

The previous comment didn't do this very well, so here's an attempt:

// Slots can only be allocated out of order on objects in
// dictionary mode.  The slot span of objects not in
// dictionary mode is computed based on the maybeSlot() of
// their last shape, so if a later shape has an earlier
// slot then the slot span will be computed incorrectly.

@@ +370,5 @@
>                        parent->hasMissingSlot() ||
> +                      child.slot() == parent->maybeSlot() + 1 ||
> +                      (parent->maybeSlot() + 1 < JSSLOT_FREE(obj->getClass()) &&
> +                       child.slot() == JSSLOT_FREE(obj->getClass()) &&
> +                       obj->is<ErrorObject>()));

It should be fine to just relax the assertion to:

child.slot() > parent->maybeSlot()

Even if it isn't possible, adding multiple slots with a new child shape should still cause the slot span to be computed correctly and for the skipped slots to be filled in with undefined values.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: v2

(In reply to Brian Hackett (:bhackett) from comment #9)
> The comment needs to indicate why we're asserting what we're asserting,
> rather than spelling out the corner case found by this fuzzbug.

Sure.  But the point of the comment/assertion isn't just to check an invariant required elsewhere.  It's also to document how the engine *should* be determining child slots.  It would meet the requirements you discuss for every non-dictionary slot to be 500 greater than the previous, and the simplified assertion wouldn't fire.  We shouldn't give up the precision of asserting immediately-after, except in the specific instances that require it.

How does this patch look, in terms of not being so unwieldy?

Comment 11

[Brian Hackett [Laid off!]]

Comment on attachment 8373658 [details] [diff] [review]
v2

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

This looks good though I still think it would be better if ErrorObject wasn't mentioned in either the comment or the assertion.  If someone changes another class in the future to behave in a similar way then this assertion will fire even though the code is fine.  And the fact that someone changed another class to behave like ErrorObject isn't terribly interesting, all we'd do in response would be fixing this test to add another special case.

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8373658 [details] [diff] [review]
v2

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

Sure, OK.

::: js/src/vm/Shape.cpp
@@ +365,5 @@
>                        parent->hasMissingSlot() ||
> +                      child.slot() == parent->maybeSlot() + 1 ||
> +                      (parent->maybeSlot() + 1 < JSSLOT_FREE(obj->getClass()) &&
> +                       child.slot() == JSSLOT_FREE(obj->getClass()) &&
> +                       obj->is<ErrorObject>()));

This function perplexes me... but that's a separate issue.

Global objects can also do this, so how about

    JS_ASSERT(obj->inDictionaryMode() ||
              parent->hasMissingSlot() ||
              obj->is<ErrorObject>() ||
              obj->is<GlobalObject>() ||
              child.slot() == parent->maybeSlot() + 1);

If it hits again bogusly though we should cut our losses and weaken the assertion.

Remember we used to have that assertion that functions never return themselves, and the assertion kept accumulating cruft, until eventually we killed it? Assertions are good... when you have actual invariants to check.

Comment 13

[Brian Hackett [Laid off!]]

I think global objects always end up in dictionary mode (I don't remember why) so shouldn't trip the existing assertion.

Comment 14

[Jeff Walden [:Waldo]]

Global objects are affected, but it's pretty hard to get it to happen.  Basically the global object has to be created, function/object classes have to be initialized on it, a couple other classes *may* be initialized but need not be (and some classes must not be) (and likely order of init among them matters, to avoid conversion-to-dictionary), and you have to redefine the "eval" property.  Pretty tricky to make happen.  I did manage to trigger it in a jsapi-test, to prove to myself it was possible (initially, to prove it wasn't possible), so I included that in the ultimate landing.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d3aca06e00

Once you throw in the byzantine GlobalObject as well as Error, I'm starting to agree about not listing them all.  It was one thing for Error, which is pretty simple, and where the reserved slot that isn't a property, isn't a property in a pretty obvious/visible way.  But GlobalObject's weird because of the temporal nature of some reserved slots not holding properties.  So I removed the extra class-check and left the rest of the assert.

Agreed about this stuff being perplexing.  There's a definite need for cleanup/reorg/refactoring of all the shape stuff at some point.  Not now.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Patch as landed in inbound

I imagine fuzzers would like to see this go places.  (I haven't tried applying it to aurora/beta, but I expect backports are trivial.)  Given it's changing an assertion but not doing anything that would affect release builds, it's as safe as they come.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 724768
User impact if declined: none -- this is purely a developer/fuzzer-facing assertion fix
Testing completed (on m-c, etc.): fuzzed by gkw above, *only* loosening an existing assertion so anything working before must continue working (so no risk of breaking stuff)
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 16

[Sylvestre Ledru [:Sylvestre]]

I will be waiting for the commit in m-c before uplifting it.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2409805b094d
https://hg.mozilla.org/releases/mozilla-beta/rev/56bd156c98eb

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/56bd156c98eb

Comment 20

[Christian Holler (:decoder)]

Comment on attachment 8373658 [details] [diff] [review]
v2

Canceling feedback here as this has already landed and is being tested as part of regular m-c fuzzing :)