Closed Bug 1236801 Opened 4 years ago Closed 4 years ago

Assertion failure: generation == table_->generation(), at dist/include/js/HashTable.h:826

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: gkw, Assigned: jimb)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:])

Attachments

(2 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision 0771c5eab32f (build with --enable-debug --enable-more-deterministic --32, run with --fuzzing-safe --no-threads --ion-eager --no-fpu):

// Adapted from randomly chosen test: js/src/jit-test/tests/debug/Debugger-onNewGlobalObject-08.js
var dbg = new Debugger;
dbg.onNewGlobalObject = function() {
    depth++;
    if (depth < 3) {
        newGlobal();
    }
    depth--;
}
var depth = 0;
// Adapted from randomly chosen test: js/src/jit-test/tests/gc/bug-1226896.js
eval(function(){}, newGlobal());
eval("oomTest((function(){return newGlobal()}))", newGlobal());

Backtrace:

0   js-dbg-32-dm-darwin-0771c5eab32f	0x0076e176 js::detail::HashTable<js::HashMapEntry<JS::Zone*, unsigned long>, js::HashMap<JS::Zone*, unsigned long, js::DefaultHasher<JS::Zone*>, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::Ptr::found() const + 198 (HashTable.h:826)
1   js-dbg-32-dm-darwin-0771c5eab32f	0x0078472d js::HashMap<JS::Zone*, unsigned long, js::DefaultHasher<JS::Zone*>, js::RuntimeAllocPolicy>::lookupWithDefault(JS::Zone* const&, unsigned long const&) + 93 (HashTable.h:247)
2   js-dbg-32-dm-darwin-0771c5eab32f	0x00785681 bool js::DebuggerWeakMap<JSObject*, false>::relookupOrAdd<JS::Rooted<JSObject*>, js::NativeObject*>(js::detail::HashTable<js::HashMapEntry<js::RelocatablePtr<JSObject*>, js::RelocatablePtr<JSObject*> >, js::HashMap<js::RelocatablePtr<JSObject*>, js::RelocatablePtr<JSObject*>, js::MovableCellHasher<js::RelocatablePtr<JSObject*> >, js::RuntimeAllocPolicy>::MapHashPolicy, js::RuntimeAllocPolicy>::AddPtr&, JS::Rooted<JSObject*> const&, js::NativeObject* const&) + 193 (Debugger.h:170)
3   js-dbg-32-dm-darwin-0771c5eab32f	0x00727424 js::Debugger::wrapDebuggeeValue(JSContext*, JS::MutableHandle<JS::Value>) + 1684 (jshashutil.h:38)
4   js-dbg-32-dm-darwin-0771c5eab32f	0x0072ca2f js::Debugger::fireNewGlobalObject(JSContext*, JS::Handle<js::GlobalObject*>, JS::MutableHandle<JS::Value>) + 207 (Debugger.cpp:1633)
5   js-dbg-32-dm-darwin-0771c5eab32f	0x0072cf38 js::Debugger::slowPathOnNewGlobalObject(JSContext*, JS::Handle<js::GlobalObject*>) + 520 (Debugger.cpp:1701)
6   js-dbg-32-dm-darwin-0771c5eab32f	0x005aa56e JS_FireOnNewGlobalObject(JSContext*, JS::Handle<JSObject*>) + 110 (RootingAPI.h:719)
7   js-dbg-32-dm-darwin-0771c5eab32f	0x0000d11c NewGlobalObject(JSContext*, JS::CompartmentOptions&, JSPrincipals*) + 1420 (js.cpp:6100)
8   js-dbg-32-dm-darwin-0771c5eab32f	0x000177eb NewGlobal(JSContext*, unsigned int, JS::Value*) + 859 (js.cpp:4008)
9   ???                           	0x01cf0966 0 + 30345574
10  ???                           	0x02e23ab0 0 + 48380592
11  ???                           	0x01ce880c 0 + 30312460
12  js-dbg-32-dm-darwin-0771c5eab32f	0x001c5ccc EnterBaseline(JSContext*, js::jit::EnterJitData&) + 620 (BaselineJIT.cpp:137)
13  js-dbg-32-dm-darwin-0771c5eab32f	0x001c5869 js::jit::EnterBaselineMethod(JSContext*, js::RunState&) + 249 (BaselineJIT.cpp:173)
14  js-dbg-32-dm-darwin-0771c5eab32f	0x007ef27f js::RunScript(JSContext*, js::RunState&) + 335 (Interpreter.cpp:397)
15  js-dbg-32-dm-darwin-0771c5eab32f	0x008090d1 js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 737 (Interpreter.cpp:478)
16  js-dbg-32-dm-darwin-0771c5eab32f	0x008096bd js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) + 381 (Interpreter.cpp:512)
17  js-dbg-32-dm-darwin-0771c5eab32f	0x005ae24c JS_CallFunction(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSFunction*>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) + 540 (jsapi.cpp:2832)
18  js-dbg-32-dm-darwin-0771c5eab32f	0x007d7968 OOMTest(JSContext*, unsigned int, JS::Value*) + 776 (TestingFunctions.cpp:1167)
19  js-dbg-32-dm-darwin-0771c5eab32f	0x0082f25d js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 221 (jscntxtinlines.h:236)
20  js-dbg-32-dm-darwin-0771c5eab32f	0x0080911e js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 814 (Interpreter.cpp:448)
21  js-dbg-32-dm-darwin-0771c5eab32f	0x008096bd js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) + 381 (Interpreter.cpp:512)
22  js-dbg-32-dm-darwin-0771c5eab32f	0x001b5841 js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 2721 (BaselineIC.cpp:6184)
23  ???                           	0x01cee7ae 0 + 30336942
24  ???                           	0x02e23950 0 + 48380240
25  ???                           	0x01ce880c 0 + 30312460
26  js-dbg-32-dm-darwin-0771c5eab32f	0x001c5ccc EnterBaseline(JSContext*, js::jit::EnterJitData&) + 620 (BaselineJIT.cpp:137)
27  js-dbg-32-dm-darwin-0771c5eab32f	0x001c5869 js::jit::EnterBaselineMethod(JSContext*, js::RunState&) + 249 (BaselineJIT.cpp:173)
28  js-dbg-32-dm-darwin-0771c5eab32f	0x007ef27f js::RunScript(JSContext*, js::RunState&) + 335 (Interpreter.cpp:397)
29  js-dbg-32-dm-darwin-0771c5eab32f	0x0080a3d7 js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) + 887 (Interpreter.h:190)
30  js-dbg-32-dm-darwin-0771c5eab32f	0x00173c32 EvalKernel(JSContext*, JS::CallArgs const&, EvalType, js::AbstractFramePtr, JS::Handle<JSObject*>, unsigned char*) + 2578 (Eval.cpp:333)
31  js-dbg-32-dm-darwin-0771c5eab32f	0x00174451 js::DirectEval(JSContext*, JS::CallArgs const&) + 865 (RootingAPI.h:719)
32  js-dbg-32-dm-darwin-0771c5eab32f	0x001b56b9 js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 2329 (BaselineIC.cpp:6169)
33  ???                           	0x01cee7ae 0 + 30336942
34  ???                           	0x02e22350 0 + 48374608
35  ???                           	0x01ce880c 0 + 30312460
36  js-dbg-32-dm-darwin-0771c5eab32f	0x001c5ccc EnterBaseline(JSContext*, js::jit::EnterJitData&) + 620 (BaselineJIT.cpp:137)
37  js-dbg-32-dm-darwin-0771c5eab32f	0x001c5869 js::jit::EnterBaselineMethod(JSContext*, js::RunState&) + 249 (BaselineJIT.cpp:173)
38  js-dbg-32-dm-darwin-0771c5eab32f	0x007ef27f js::RunScript(JSContext*, js::RunState&) + 335 (Interpreter.cpp:397)
39  js-dbg-32-dm-darwin-0771c5eab32f	0x0080a3d7 js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) + 887 (Interpreter.h:190)
40  js-dbg-32-dm-darwin-0771c5eab32f	0x0080a7c4 js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) + 484 (RootingAPI.h:719)
41  js-dbg-32-dm-darwin-0771c5eab32f	0x005b3c75 ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) + 405 (jsapi.cpp:4340)
42  js-dbg-32-dm-darwin-0771c5eab32f	0x005b3f36 JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) + 86 (RootingAPI.h:719)
43  js-dbg-32-dm-darwin-0771c5eab32f	0x00021929 Process(JSContext*, char const*, bool, FileKind) + 3401 (js.cpp:516)
44  js-dbg-32-dm-darwin-0771c5eab32f	0x0000673f main + 13135 (js.cpp:6219)
45  js-dbg-32-dm-darwin-0771c5eab32f	0x000025e5 start + 53
Since this is in Debugger-land, I'm setting a needinfo? from :jimb and :fitzgen.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
I can reproduce under RR.
Assignee: nobody → jimb
Flags: needinfo?(jimb)
Flags: needinfo?(nfitzgerald)
So what's going on here is a bug in js/public/HashTable.h's simulated OOM handling; Debugger is just the victim.

First, note that testing an AddPtr as a boolean value requires that the AddPtr's generation match that of its table:
https://hg.mozilla.org/mozilla-central/file/0771c5eab32f/js/public/HashTable.h#l826

Second, note that if a simulated OOM is requested by the call to checkSimulatedOOM here:
https://hg.mozilla.org/mozilla-central/file/0771c5eab32f/js/public/HashTable.h#l1660
then we're in an odd situation, because the call to checkOverloaded just above has bumped the table's generation, which it does only on a successful resize; yet we have not reached the update to p.generation just below. Hence, the simulated OOM can leave the AddPtr in an untestable state.

(Note that a real OOM can't do this: if checkOverloaded fails, it doesn't update the table's generation, so the AddPtr remains testable.)

This can then cause the assertion here to fail:
https://hg.mozilla.org/mozilla-central/file/0771c5eab32f/js/public/HashTable.h#l247
because the call to add failed, and left p in an untestable state.

Would it be sufficient to move the update to p.generation earlier in HashTable::add, so that it occurs whenever the real resize succeeds?

Or would it be better to move the checkSimulatedOOM nearer the actual allocation in checkOverloaded, so that we never have the situation where checkOverloaded succeeds, but add fails?
Flags: needinfo?(jcoppeard)
Thanks for analysing this.  You're right, the simulated OOM handling is at fault here.

I think think the solution is not to simulate OOM if we already resized the table.  This is unnecessary anyway because all the allocation function can also simulate OOM.
Assignee: jimb → jcoppeard
Flags: needinfo?(jcoppeard)
Assignee: jcoppeard → jimb
Status: NEW → ASSIGNED
Attachment #8705745 - Flags: review?(jcoppeard)
Comment on attachment 8705745 [details] [diff] [review]
Don't check for simulated OOM at a point where the code can't handle an abrupbt exit.

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

Oh I see you got to this before I could write a patch!

::: js/public/HashTable.h
@@ -1657,5 @@
>              RebuildStatus status = checkOverloaded();
>              if (status == RehashFailed)
>                  return false;
> -            if (!this->checkSimulatedOOM())
> -                return false;

This wasn't quite what I meant though.  We still want to potentially simulate OOM if the table isn't resized.  I was thinking something like:

    if (status == NotOverloaded && !this->checkSimulatedOOM())	
        return false;
Attachment #8705745 - Flags: review?(jcoppeard)
(In reply to Jon Coppeard (:jonco) from comment #8)
> This wasn't quite what I meant though.  We still want to potentially
> simulate OOM if the table isn't resized.

So it's intentional that we make operations that have no excuse to OOM do so anyway? Hmm, okay.
I've verified that the test in this patch fails without the change to HashTable.js.
Attachment #8705745 - Attachment is obsolete: true
Attachment #8705887 - Flags: review?(jcoppeard)
... and that it fails the same assertion as the original test case, but much more quickly.
(In reply to Jim Blandy :jimb from comment #9)

> So it's intentional that we make operations that have no excuse to OOM do so
> anyway? Hmm, okay.

Yes, because we want to be able to test that all calls to an API that can allocate memory handle OOM correctly, even if the API doesn't allocate on every call.
Comment on attachment 8705887 [details] [diff] [review]
Don't check for simulated OOM in a way that invalidates AddPtrs for no discernable reason.

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

Great, thank you for fixing this and adding test code.
Attachment #8705887 - Flags: review?(jcoppeard) → review+
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla46
https://hg.mozilla.org/mozilla-central/rev/f46824dc517a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Depends on: 1254123
You need to log in before you can comment on or make changes to this bug.