1103386 - Assertion failure: !has, at vm/DebuggerMemory.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

// Random chosen test: js/src/jit-test/tests/auto-regress/bug700295.js
__proto__ = null
Object.prototype.__proto__ = this
// Random chosen test: js/src/jit-test/tests/debug/Memory-takeCensus-05.js
Debugger(newGlobal()).memory.takeCensus()

asserts js debug shell on m-c changeset 7ab92d922d19 with --no-ion --no-threads at Assertion failure: !has, at vm/DebuggerMemory.cpp.

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 file(s) is/are:

http://hg.mozilla.org/mozilla-central/file/7ab92d922d19/js/src/jit-test/tests/auto-regress/bug700295.js
http://hg.mozilla.org/mozilla-central/file/7ab92d922d19/js/src/jit-test/tests/debug/Memory-takeCensus-05.js

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/c6c02127a3ce
user:        Jim Blandy
date:        Mon Aug 11 12:46:39 2014 -0700
summary:     Bug 1012456: Add non-trivial breakdowns to Debugger.Memory.prototype.takeCensus. r=terrence

Jim, is bug 1012456 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x34be41, 0x00000001006b036d js-dbg-opt-64-dm-nsprBuild-darwin-7ab92d922d19`js::dbg::ByObjectClass<js::dbg::Tally, js::dbg::Tally>::report(js::dbg::Census&, JS::MutableHandle<JS::Value>) [inlined] js::detail::HashTable<js::HashMapEntry<js::Class const*, js::dbg::Tally>, js::HashMap<js::Class const*, js::dbg::Tally, js::dbg::ByObjectClass<js::dbg::Tally, js::dbg::Tally>::HashPolicy, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::count() const + 28 at HashTable.h:1519, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001006b036d js-dbg-opt-64-dm-nsprBuild-darwin-7ab92d922d19`js::dbg::ByObjectClass<js::dbg::Tally, js::dbg::Tally>::report(js::dbg::Census&, JS::MutableHandle<JS::Value>) [inlined] js::detail::HashTable<js::HashMapEntry<js::Class const*, js::dbg::Tally>, js::HashMap<js::Class const*, js::dbg::Tally, js::dbg::ByObjectClass<js::dbg::Tally, js::dbg::Tally>::HashPolicy, js::SystemAllocPolicy>::MapHashPolicy, js::SystemAllocPolicy>::count() const + 28 at HashTable.h:1519
    frame #1: 0x00000001006b0351 js-dbg-opt-64-dm-nsprBuild-darwin-7ab92d922d19`js::dbg::ByObjectClass<js::dbg::Tally, js::dbg::Tally>::report(js::dbg::Census&, JS::MutableHandle<JS::Value>) [inlined] js::HashMap<js::Class const*, js::dbg::Tally, js::dbg::ByObjectClass<js::dbg::Tally, js::dbg::Tally>::HashPolicy, js::SystemAllocPolicy>::count(this=<unavailable>) const at HashTable.h:192
    frame #2: 0x00000001006b0351 js-dbg-opt-64-dm-nsprBuild-darwin-7ab92d922d19`js::dbg::ByObjectClass<js::dbg::Tally, js::dbg::Tally>::report(this=<unavailable>, census=<unavailable>, report=<unavailable>) + 1745 at DebuggerMemory.cpp:548
    frame #3: 0x00000001006af8da js-dbg-opt-64-dm-nsprBuild-darwin-7ab92d922d19`js::dbg::ByJSType<js::dbg::ByObjectClass<js::dbg::Tally, js::dbg::Tally>, js::dbg::Tally, js::dbg::Tally, js::dbg::ByUbinodeType<js::dbg::Tally> >::report(this=0x00007fff5fbfe100, census=0x00007fff5fbfe1d0, report=<unavailable>) + 314 at DebuggerMemory.cpp:443
    frame #4: 0x000000010063e261 js-dbg-opt-64-dm-nsprBuild-darwin-7ab92d922d19`js::DebuggerMemory::takeCensus(JSContext*, unsigned int, JS::Value*) [inlined] JS::detail::CallReceiverBase<(this=0x00007fff5fbfe1d0, census=0x0000000101b14ca0)0>::rval() const + 8 at DebuggerMemory.cpp:703
(lldb)

Comment 2

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

Also setting needinfo? from :fitzgen since this involves Debugger.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I can reproduce, but I don't really understand what invariants this assert is trying to maintain. Jim, wrote this code, so hopefully he knows.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I think we could fix this if the `report` method created the equivalent of `Object.create(null)` rather than `{}`. Is that sane, Jim?

Comment 5

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> I think we could fix this if the `report` method created the equivalent of
> `Object.create(null)` rather than `{}`. Is that sane, Jim?

Yes, that's definitely preferable.

Comment 6

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #5)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> > I think we could fix this if the `report` method created the equivalent of
> > `Object.create(null)` rather than `{}`. Is that sane, Jim?
> 
> Yes, that's definitely preferable.

We should be using this in all the 'report' member functions, I think.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

The other thing we could do is use js::HasOwnProperty instead of JSObject::hasProperty. That way, when people aren't doing insane things like this test case, they still get a JSON.stringify-able and toString-able object.

Comment 8

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

Just wondering, what's next here?

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Jim, are you going to cook up a patch? Or do you want me to take this one?

Comment 10

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> The other thing we could do is use js::HasOwnProperty instead of
> JSObject::hasProperty. That way, when people aren't doing insane things like
> this test case, they still get a JSON.stringify-able and toString-able
> object.

Yeah, I like this solution a lot better. Good thinking.

Comment 11

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> Jim, are you going to cook up a patch? Or do you want me to take this one?

I have a bunch of big reviews I need to get through, so it'd be a while before I could look at it. I'd be happy to review a patch with tests, though!

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: census-has-assertion-failure.patch

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8545374 [details] [diff] [review]
census-has-assertion-failure.patch

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

::: js/src/vm/DebuggerMemory.cpp
@@ +568,5 @@
>              // example, JSObject::class_, Debugger.Object, and CollatorClass are
>              // all "Object"), so let's make sure our hash table treats them all
>              // as equivalent.
>              bool has;
> +            if (!HasOwnProperty(cx, obj, entryId, &has))

The bug is that NewBuiltinClassInstance<PlainObject> is vulnerable to [[Prototype]] poisoning, so we should use HasOwnProperty? I don't really understand what's going on here.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

ni? me to check this in once trees open up again

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Finally, inbound is open!

https://hg.mozilla.org/integration/mozilla-inbound/rev/25b04c1440c8

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/25b04c1440c8