1101561 - Assertion failure: isObject(), at dist/include/js/Value.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

// Random chosen test: js/src/jit-test/tests/basic/bug858097.js
gcparam("maxBytes", gcparam("gcBytes") + 4 * (1));
gczeal(4);
// Random chosen test: js/src/jit-test/tests/basic/__proto__-not-prototype-mutation.js
try {
    for (var y of [{}]) {}
} catch (e) {}
// Random chosen test: js/src/jit-test/tests/auto-regress/bug589093.js
x = (w for (x in []))
// jsfunfuzz
function e()
function()
function()(function() {
    [1] / function()
    function()
    function()
    function()
    function()
    function() {
        ""
        (function() {})
    }
    function n()
    ([{
            n() {}
        }, {
            n() {}
        }, {
            n() {}
        }, {
            n() {}
        }, {
            n() {}
        }, {
            n() {}
        }, {
            n() {}
        }, {
            n() {}
        }
    ]);
    t = function() {}
})(function() {
    function c() {}
})
function n() n = {
    n() {
        return {
            n() {}
        }
    }, n() {}
}
function t()
function()
function()
function() {
    [""]
    function e() {}
}
function f() {
    e(/x/, function() {})
}

asserts js debug shell on m-c changeset d0d8c407efb5 with --fuzzing-safe --no-threads --no-baseline --no-ion at Assertion failure: isObject(), at dist/include/js/Value.h.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

This was found by combining random jit-tests together with jsfunfuzz, the specific files are:

http://hg.mozilla.org/mozilla-central/file/d0d8c407efb5/js/src/jit-test/tests/basic/bug858097.js
http://hg.mozilla.org/mozilla-central/file/d0d8c407efb5/js/src/jit-test/tests/basic/__proto__-not-prototype-mutation.js
http://hg.mozilla.org/mozilla-central/file/d0d8c407efb5/js/src/jit-test/tests/auto-regress/bug589093.js

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20141029053910" and the hash "f7c01f0ac64b".
The "bad" changeset has the timestamp "20141029055110" and the hash "75de7e0fe086".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f7c01f0ac64b&tochange=75de7e0fe086

Jan, is bug 1090491 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x13ff12, 0x000000010001c0f2 js-dbg-opt-64-dm-nsprBuild-darwin-d0d8c407efb5`JS::Value::toObject() const [inlined] JSVAL_IS_OBJECT_IMPL(jsval_layout) + 28 at Value.h:807, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x000000010001c0f2 js-dbg-opt-64-dm-nsprBuild-darwin-d0d8c407efb5`JS::Value::toObject() const [inlined] JSVAL_IS_OBJECT_IMPL(jsval_layout) + 28 at Value.h:807
    frame #1: 0x000000010001c0d6 js-dbg-opt-64-dm-nsprBuild-darwin-d0d8c407efb5`JS::Value::toObject() const [inlined] JS::Value::isObject(this=<unavailable>) const at Value.h:1148
    frame #2: 0x000000010001c0d6 js-dbg-opt-64-dm-nsprBuild-darwin-d0d8c407efb5`JS::Value::toObject(this=<unavailable>) const + 182 at Value.h:1243
    frame #3: 0x000000010068a459 js-dbg-opt-64-dm-nsprBuild-darwin-d0d8c407efb5`js::GlobalObject::getOrCreateLegacyGeneratorObjectPrototype(cx=<unavailable>, global=<unavailable>) + 105 at GlobalObject.h:481
    frame #4: 0x000000010063ca98 js-dbg-opt-64-dm-nsprBuild-darwin-d0d8c407efb5`js::GeneratorObject::create(cx=0x0000000101c01d00, frame=(ptr_ = 4337021657)) + 2488 at GeneratorObject.cpp:42
(lldb)

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] GMT+8 after Oct 9 from comment #0)
> Jan, is bug 1090491 a likely regressor?

No, this is an older OOM issue.

The problem is that we OOM in GlobalObject::initGeneratorClasses, here:

    if (!proto || !DefinePropertiesAndFunctions(cx, proto, nullptr, legacy_generator_methods))
        return false;
    global->setReservedSlot(LEGACY_GENERATOR_OBJECT_PROTO, ObjectValue(*proto));

This throws ("out of memory" string) but this exception is caught by the try-catch. Then later on we call GeneratorObject::create, where we do:

    JSObject *proto = GlobalObject::getOrCreateLegacyGeneratorObjectPrototype(cx, global);

And this asserts because it expects an object in the LEGACY_GENERATOR_OBJECT_PROTO slot. Waldo, do you know how all this is supposed to work?

Comment 3

[Christian Holler (:decoder)]

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

Comment 4

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

OOM bugs can be fragile. This issue seems diagnosed in comment 2, so no need for tracking via JSBugMon.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Fix StopIteration

Here come a handful of patches to make iteration class init OOM-safe, approximately one thing at a time, for greatest readability.  Beware, the last one is fugly as all sin.  :-(

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Fix %ArrayIteratorPrototype% initialization

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Fix %StringIteratorPrototype% init

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Fix %IteratorPrototype% init

The whitespace-mangling of this isn't horribly great, alas.  I *think* bzexport has a -w mode, will post that for additional perusal

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Fix %IteratorPrototype% init (-w version)

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Fix generator bootstrapping (for legacy and star generators both)

The transliterate function is truly grody, but I need *something* on GlobalObject to get access to slot numbers, and this is not irredeemably ugly, at least.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8664667 [details] [diff] [review]
Fix %ArrayIteratorPrototype% initialization

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

::: js/src/jsiter.cpp
@@ +1377,5 @@
> +{
> +    if (global->getReservedSlot(ARRAY_ITERATOR_PROTO).isObject())
> +        return true;
> +
> +    Rooted<JSObject*> iteratorProto(cx, GlobalObject::getOrCreateIteratorPrototype(cx, global));

Nit: this method uses both Rooted<JSObject*> and RootedObject; let's pick one.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8664669 [details] [diff] [review]
Fix %IteratorPrototype% init

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

::: js/src/jsprototypes.h
@@ +75,5 @@
>      real(ReferenceError,        15,     InitViaClassSpec,       ERROR_CLASP(JSEXN_REFERENCEERR)) \
>      real(SyntaxError,           16,     InitViaClassSpec,       ERROR_CLASP(JSEXN_SYNTAXERR)) \
>      real(TypeError,             17,     InitViaClassSpec,       ERROR_CLASP(JSEXN_TYPEERR)) \
>      real(URIError,              18,     InitViaClassSpec,       ERROR_CLASP(JSEXN_URIERR)) \
> +    real(Iterator,              19,     InitIteratorClass,    OCLASP(PropertyIterator)) \

Nit: fix last column to line up with previous row.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8664674 [details] [diff] [review]
Fix generator bootstrapping (for legacy and star generators both)

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

I expected the patch to be worse, after I read your description :) Thanks for dismantling initIteratorClasses, this is much saner.

::: js/src/jsprototypes.h
@@ +96,5 @@
>      real(Symbol,                36,     InitSymbolClass,        OCLASP(Symbol)) \
>  IF_SAB(real,imaginary)(SharedArrayBuffer,       37,     InitSharedArrayBufferClass, &js::SharedArrayBufferObject::protoClass) \
>  IF_INTL(real,imaginary) (Intl,                  38,     InitIntlClass,          CLASP(Intl)) \
>  IF_BDATA(real,imaginary)(TypedObject,           39,     InitTypedObjectModuleObject,   OCLASP(TypedObjectModule)) \
> +    real(Reflect,               40,      InitReflect,           nullptr) \

Hm nevermind the comment on the last patch; you're replacing that line here anyway.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ead219581dbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/8add4271e98c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b09e2a11f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3138a3efe4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/387c21d5c4e3

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Move ensuring classes needed by parsing into a single method, and assert all needed prototypes are created as delegates

Turns out I missed a spot in HelperThreads.cpp where eagerly-init-various-standard-classes occurs.  Time to combine all three (count 'em) spots we do that into one helper method.

Also, HT.cpp's EnsureConstructor makes everything a delegate, likely as objects with non-delegate prototypes would be Bad.  But most standard class prototypes are created by createBlankPrototype*, which already does this marking.  And there's not too many others, needed in these cases, that aren't marked as such.  So just eagerly do that to them, and change the setDelegate to an assertion.

As for the standard classes affected by this:

* Object.prototype is proto of everything, so already a delegate
* Function.prototype is proto of functions, so already a delegate
* Array.prototype I just fixed
* RegExp.prototype is created via createBlankPrototype, so okay
* Iterator.prototype is created via cBP, so okay
* legacy iterator prototype I fix here
* star generator object/function protos I fix here

And that completes the list.  (Although I'm not sure Iterator.prototype really needs to be in this list, early-created.  But for a followup.)

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8665846 [details] [diff] [review]
Move ensuring classes needed by parsing into a single method, and assert all needed prototypes are created as delegates

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

Beautiful!

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> Also, HT.cpp's EnsureConstructor makes everything a delegate, likely as
> objects with non-delegate prototypes would be Bad.

FWIW I think I added those setDelegate calls when we failed the isDelegate assertion added to setProtoUnchecked. Teleporting, mutable __proto__ etc depend on isDelegate being correct.

::: js/src/vm/HelperThreads.cpp
@@ +328,5 @@
> +// data structures, template objects, &c.
> +static bool
> +EnsureParserCreatedClasses(JSContext* cx)
> +{
> +    Rooted<GlobalObject*> global(cx, &cx->global()->as<GlobalObject>());

Nit: global() returns a Handle<GlobalObject*> so I think we can remove the as<...> here. Or avoid the Rooted if you want:

Handle<GlobalObject*> global = cx->global();

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9174682898d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c10da22ca197
https://hg.mozilla.org/integration/mozilla-inbound/rev/d06ebf9bb7ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/b62b29522364
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b80aed590fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/1415320cf150

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9174682898d4
https://hg.mozilla.org/mozilla-central/rev/c10da22ca197
https://hg.mozilla.org/mozilla-central/rev/d06ebf9bb7ca
https://hg.mozilla.org/mozilla-central/rev/b62b29522364
https://hg.mozilla.org/mozilla-central/rev/7b80aed590fb
https://hg.mozilla.org/mozilla-central/rev/1415320cf150