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

I used the tips in: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips#How_to_debug_oomTest%28%29_failures and got a stack which I've attached.

Since this is in Debugger-land, I'm setting a needinfo? from :jimb and :fitzgen.

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

I can reproduce under RR.

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?

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.

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;

(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.

... 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.

Had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6491f2e5d963 for SM build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=19703946&repo=mozilla-inbound