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

RESOLVED FIXED in Firefox 28, Firefox OS v1.3

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gkw, Assigned: Waldo)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla30
x86_64
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox28 fixed, firefox29 fixed, firefox30 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8362250 [details]
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>
(Reporter)

Comment 1

4 years ago
Created attachment 8362334 [details]
regression window for threadsafe builds

Bug 724768 seems to involve the Error object and is in the regression window, setting needinfo? from Waldo.
Flags: needinfo?(jwalden+bmo)
(Reporter)

Updated

4 years ago
Blocks: 724768
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.
Flags: needinfo?(jwalden+bmo)
(Reporter)

Comment 3

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

Assigning since Waldo is on the case.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Created 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

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.
Attachment #8371824 - Flags: review?(jorendorff)
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.
Attachment #8371824 - Flags: feedback?(bhackett1024)
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!
Attachment #8371824 - Flags: feedback?(gary)
(Reporter)

Updated

4 years ago
Attachment #8371824 - Flags: feedback?(choller)
(Reporter)

Updated

4 years ago
Depends on: 969712
(Reporter)

Comment 7

4 years ago
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.
Attachment #8371824 - Flags: feedback?(choller) → feedback+
(Reporter)

Comment 8

4 years ago
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.
Attachment #8371824 - Flags: feedback?(gary) → feedback?(choller)
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.
Attachment #8371824 - Flags: feedback?(bhackett1024)
Created attachment 8373658 [details] [diff] [review]
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?
Attachment #8371824 - Attachment is obsolete: true
Attachment #8371824 - Flags: review?(jorendorff)
Attachment #8371824 - Flags: feedback?(choller)
Attachment #8373658 - Flags: review?(jorendorff)
Attachment #8373658 - Flags: feedback?(choller)
Attachment #8373658 - Flags: feedback?(bhackett1024)
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.
Attachment #8373658 - Flags: feedback?(bhackett1024) → feedback+
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.
Attachment #8373658 - Flags: review?(jorendorff) → review+
I think global objects always end up in dictionary mode (I don't remember why) so shouldn't trip the existing assertion.
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.
Created attachment 8375243 [details] [diff] [review]
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
Attachment #8375243 - Flags: review+
Attachment #8375243 - Flags: approval-mozilla-beta?
Attachment #8375243 - Flags: approval-mozilla-aurora?
I will be waiting for the commit in m-c before uplifting it.
https://hg.mozilla.org/mozilla-central/rev/c2d3aca06e00
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Attachment #8375243 - Flags: approval-mozilla-beta?
Attachment #8375243 - Flags: approval-mozilla-beta+
Attachment #8375243 - Flags: approval-mozilla-aurora?
Attachment #8375243 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2409805b094d
https://hg.mozilla.org/releases/mozilla-beta/rev/56bd156c98eb
status-firefox28: --- → fixed
status-firefox29: --- → fixed
status-firefox30: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/56bd156c98eb
status-b2g-v1.3: --- → fixed
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 :)
Attachment #8373658 - Flags: feedback?(choller)
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed
You need to log in before you can comment on or make changes to this bug.