Closed Bug 1026139 Opened 5 years ago Closed 5 years ago

crash in DebugModeOSREntry::DebugModeOSREntry(JSScript*, js::jit::ICEntry const&)

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

32 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox30 --- unaffected
firefox31 --- wontfix
firefox32 + fixed
firefox33 --- fixed

People

(Reporter: jbecerra, Assigned: shu)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-f4ecabb2-a924-4f9f-80e7-8a8cc2140610.
=============================================================

This is a recent top crasher in Fx32 Aurora. It looks like it started happening on 5/22, but it has had a recent spike. Mostly on Win7 and 8/8.1. There are a few comments but not much to go by. 

More reports at: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=DebugModeOSREntry%3A%3ADebugModeOSREntry%28JSScript%2A%2C+js%3A%3Ajit%3A%3AICEntry+const%26%29

0 	mozjs.dll 	DebugModeOSREntry::DebugModeOSREntry(JSScript *,js::jit::ICEntry const &) 	js/src/jit/BaselineDebugModeOSR.cpp
1 	mozjs.dll 	CollectOnStackScripts 	js/src/jit/BaselineDebugModeOSR.cpp
2 	mozjs.dll 	js::jit::RecompileOnStackBaselineScriptsForDebugMode(JSContext *,JSCompartment *) 	js/src/jit/BaselineDebugModeOSR.cpp
3 	mozjs.dll 	js::jit::UpdateForDebugMode(JSContext *,JSCompartment *,js::AutoDebugModeInvalidation &) 	js/src/jit/Ion.cpp
4 	mozjs.dll 	JSCompartment::updateJITForDebugMode(JSContext *,js::AutoDebugModeInvalidation &) 	js/src/jscompartment.cpp
5 	mozjs.dll 	JSCompartment::removeDebuggee(JSContext *,js::GlobalObject *,js::AutoDebugModeInvalidation &,js::detail::HashTable<js::GlobalObject * const,js::HashSet<js::GlobalObject *,js::DefaultHasher<js::GlobalObject *>,js::SystemAllocPolicy>::SetOps,js::SystemAllocPolicy>::Enum *) 	js/src/jscompartment.cpp
6 	mozjs.dll 	js::Debugger::removeDebuggeeGlobal(JSContext *,js::GlobalObject *,js::AutoDebugModeInvalidation &,js::detail::HashTable<js::GlobalObject * const,js::HashSet<js::GlobalObject *,js::DefaultHasher<js::GlobalObject *>,js::SystemAllocPolicy>::SetOps,js::SystemAllocPolicy>::Enum *,js::detail::HashTable<js::GlobalObject * const,js::HashSet<js::GlobalObject *,js::DefaultHasher<js::GlobalObject *>,js::SystemAllocPolicy>::SetOps,js::SystemAllocPolicy>::Enum *) 	js/src/vm/Debugger.cpp
7 	mozjs.dll 	js::Debugger::removeDebuggeeGlobal(JSContext *,js::GlobalObject *,js::detail::HashTable<js::GlobalObject * const,js::HashSet<js::GlobalObject *,js::DefaultHasher<js::GlobalObject *>,js::SystemAllocPolicy>::SetOps,js::SystemAllocPolicy>::Enum *,js::detail::HashTable<js::GlobalObject * const,js::HashSet<js::GlobalObject *,js::DefaultHasher<js::GlobalObject *>,js::SystemAllocPolicy>::SetOps,js::SystemAllocPolicy>::Enum *) 	js/src/vm/Debugger.cpp
8 	mozjs.dll 	js::Debugger::removeAllDebuggees(JSContext *,unsigned int,JS::Value *) 	js/src/vm/Debugger.cpp
9 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
10 	mozjs.dll 	js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
11 	mozjs.dll 	js::CrossCompartmentWrapper::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) 	js/src/jswrapper.cpp
12 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
13 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp
14 	mozjs.dll 	js::RunScript(JSContext *,js::RunState &) 	js/src/vm/Interpreter.cpp
15 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
16 	mozjs.dll 	js::CallOrConstructBoundFunction(JSContext *,unsigned int,JS::Value *) 	js/src/jsfun.cpp
17 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
18 	mozjs.dll 	js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
19 	mozjs.dll 	js::CrossCompartmentWrapper::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) 	js/src/jswrapper.cpp
20 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
21 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp
22 	mozjs.dll 	js::RunScript(JSContext *,js::RunState &) 	js/src/vm/Interpreter.cpp
23 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
24 	mozjs.dll 	js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
25 	mozjs.dll 	js::CrossCompartmentWrapper::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) 	js/src/jswrapper.cpp
26 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
27 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp
28 	mozjs.dll 	js::RunScript(JSContext *,js::RunState &) 	js/src/vm/Interpreter.cpp
29 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
30 	mozjs.dll 	js_fun_apply(JSContext *,unsigned int,JS::Value *) 	js/src/jsfun.cpp
31 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
32 	mozjs.dll 	js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
33 	mozjs.dll 	js::CrossCompartmentWrapper::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) 	js/src/jswrapper.cpp
34 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
35 	mozjs.dll 	Interpret 	js/src/vm/Interpreter.cpp
36 	mozjs.dll 	js::RunScript(JSContext *,js::RunState &) 	js/src/vm/Interpreter.cpp
37 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
38 	mozjs.dll 	js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
39 	mozjs.dll 	js::CrossCompartmentWrapper::call(JSContext *,JS::Handle<JSObject *>,JS::CallArgs const &) 	js/src/jswrapper.cpp
40 	mozjs.dll 	js::Invoke(JSContext *,JS::CallArgs,js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
41 	mozjs.dll 	js::Invoke(JSContext *,JS::Value const &,JS::Value const &,unsigned int,JS::Value const *,JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
42 	mozjs.dll 	JS_CallFunctionValue(JSContext *,JS::Handle<JSObject *>,JS::Handle<JS::Value>,JS::HandleValueArray const &,JS::MutableHandle<JS::Value>) 	js/src/jsapi.cpp
43 	xul.dll 	nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS *,unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) 	js/xpconnect/src/XPCWrappedJSClass.cpp
44 	xul.dll 	nsXPCWrappedJS::CallMethod(unsigned short,XPTMethodDescriptor const *,nsXPTCMiniVariant *) 	js/xpconnect/src/XPCWrappedJS.cpp
45 	nss3.dll 	md_UnlockAndPostNotifies 	nsprpub/pr/src/md/windows/w95cv.c
46 	nss3.dll 	_MD_CURRENT_THREAD 	nsprpub/pr/src/md/windows/w95thred.c
It looks like it's failing to find an ICEntry when turning off debug mode. Without STR it's kind of hard to say why...
The bug that this patch fixes has the same stacktrace as the crasher. Without
STR I can't guarantee it fixes the same bug, but it seems very likely.

Jan, I'm not 100% on the approach here, which basically depends on the
ICEntries vector of a Baseline script being the same across recompiles on the
same script given the same debug mode. Do you have a better suggestion?
Attachment #8441128 - Flags: review?(jdemooij)
Attached patch Test.Splinter Review
Attachment #8441129 - Flags: review?(jimb)
Assignee: nobody → shu
Status: NEW → ASSIGNED
Comment on attachment 8441128 [details] [diff] [review]
Fix patching already patched frames in debug mode OSR.

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

I'm a bit worried about how it becomes more complicated with every fix, but i don't see a good alternative. Hopefully we found most issues and this will fix the crashes :)
Attachment #8441128 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #4)
> Comment on attachment 8441128 [details] [diff] [review]
> Fix patching already patched frames in debug mode OSR.
> 
> Review of attachment 8441128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a bit worried about how it becomes more complicated with every fix, but
> i don't see a good alternative. Hopefully we found most issues and this will
> fix the crashes :)

Yeah, I worry too. If this complexity gets out of hand, I will turn off patching for on -> off transitions, and the extra complexity introduced here goes away. I do them now because I still believe in minimal code slowdown with Debugger off, but if it's a landmine to maintain, then screw it.
Comment on attachment 8441129 [details] [diff] [review]
Test.

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

::: js/src/jit-test/tests/debug/Debugger-debuggees-28.js
@@ +1,5 @@
> +// Test that on->off->on and off->on->off toggles don't crash.
> +
> +function addRemove(dbg, g) {
> +  dbg.addDebuggee(g);
> +  dbg.removeDebuggee(g);

Would it be interesting to actually do some debugger-ish stuff while we're momentarily in debug mode? Walk D.F's to the oldest on the stack, for example?

I don't understand the machinery this is exercising, so my grasp of what is valuable in a grey-boxing sense is not good.

@@ +41,5 @@
> +  });
> +
> +  assertEq(g.eval("(" + function () {
> +    return f();
> +  } + ")();"), 100);

Is there a reason this is better than simply:

assertEq(g.f(), 100)

?
Attachment #8441129 - Flags: review?(jimb) → review+
Some more testing revealed that the invariant I
relied on in the previous patch (that the ICEntry vector is constant across
recompiles of the same script for the same debug mode) doesn't actually hold,
due to things marking a script as non-Ion compileable.

This version has a simpler, better approach. If a frame alreayd has a
DebugModeOSRInfo stashed on it, just grab the pc off of that and use the
existing machinery.

Really sorry for the churn.
Attachment #8441128 - Attachment is obsolete: true
Attachment #8442649 - Flags: review?(jdemooij)
Comment on attachment 8442649 [details] [diff] [review]
Fix patching already patched frames in debug mode OSR.

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

Nice, this is simpler.
Attachment #8442649 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/8c673147b572
https://hg.mozilla.org/mozilla-central/rev/193f6829aa13
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
#12 crash on Aurora 32
Comment on attachment 8442649 [details] [diff] [review]
Fix patching already patched frames in debug mode OSR.

Approval Request Comment
[Feature/regressing bug #]: 716647
[User impact if declined]: Crashes when attempting to debug from the slow script dialog on some web pages.
[Describe test coverage new/current, TBPL]: Tested on central
[Risks and why]: Low risk for taking; buggy code paths only taken when opening the debugger from the slow script dialog, which is already an exceptional case.
[String/UUID change made/needed]: None
Attachment #8442649 - Flags: approval-mozilla-aurora?
Comment on attachment 8442649 [details] [diff] [review]
Fix patching already patched frames in debug mode OSR.

Aurora+
Attachment #8442649 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.