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

RESOLVED FIXED in mozilla38

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gkw, Assigned: fitzgen)

Tracking

(Blocks 2 bugs, {assertion, regression, testcase})

Trunk
mozilla38
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 affected)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments)

// 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?
Flags: needinfo?(jimb)
Posted file 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)
Also setting needinfo? from :fitzgen since this involves Debugger.
Flags: needinfo?(nfitzgerald)
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.
Flags: needinfo?(nfitzgerald)
I think we could fix this if the `report` method created the equivalent of `Object.create(null)` rather than `{}`. Is that sane, Jim?
(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.
Flags: needinfo?(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.
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.
Just wondering, what's next here?
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
Jim, are you going to cook up a patch? Or do you want me to take this one?
Flags: needinfo?(nfitzgerald)
(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.
Flags: needinfo?(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!
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
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.
Attachment #8545374 - Flags: review?(shu) → review+
ni? me to check this in once trees open up again
Flags: needinfo?(nfitzgerald)
Finally, inbound is open!

https://hg.mozilla.org/integration/mozilla-inbound/rev/25b04c1440c8
Flags: needinfo?(nfitzgerald)
https://hg.mozilla.org/mozilla-central/rev/25b04c1440c8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.