701299 - crash nsGlobalWindow::LeaveModalState

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Martijn Wargers (dead)]

This bug was filed from the Socorro interface and is 
report bp-9cfe03ea-8c52-4281-a7b3-275402111109 .
============================================================= 
0 	xul.dll 	nsGlobalWindow::LeaveModalState 	dom/base/nsGlobalWindow.cpp:6795
1 	xul.dll 	nsDOMWindowUtils::LeaveModalStateWithWindow 	dom/base/nsDOMWindowUtils.cpp:1530
2 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
3 	xul.dll 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1553
4 	mozjs.dll 	CallCompiler::generateNativeStub 	js/src/methodjit/MonoIC.cpp:939
5 	mozjs.dll 	js::mjit::ic::NativeCall 	js/src/methodjit/MonoIC.cpp:1173
6 	mozjs.dll 	js::mjit::EnterMethodJIT 	js/src/methodjit/MethodJIT.cpp:1064
7 	mozjs.dll 	js::mjit::JaegerShot 	js/src/methodjit/MethodJIT.cpp:1142
8 	mozjs.dll 	js::RunScript 	js/src/jsinterp.cpp:581
9 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:647
10 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:679
11 	mozjs.dll 	js::ProxyHandler::call 	js/src/jsproxy.cpp:275
12 	mozjs.dll 	js::Wrapper::call 	js/src/jswrapper.cpp:262
13 	mozjs.dll 	js::CrossCompartmentWrapper::call 	js/src/jswrapper.cpp:718
14 	mozjs.dll 	js::Proxy::call 	js/src/jsproxy.cpp:841
15 	mozjs.dll 	proxy_Call 	js/src/jsproxy.cpp:1345
16 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:622
17 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:3948
18 	mozjs.dll 	js::types::TypeSet::addType 	js/src/jsinferinlines.h:1034
19 	mozjs.dll 	js::types::TypeScript::SetThis 	js/src/jsinferinlines.h:628
20 	mozjs.dll 	js::ExecuteKernel 	js/src/jsinterp.cpp:783

Comment 1

[Sheila Mooney]

Most of these are from FF 4.0b11. Several from 10.0a1.

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

Comment 3

[Andrew Quartey [:drexler]]

Attachment: patch

Looks like null deref. It might indicate further problems afield but this nips it in the bud.

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 623450 [details] [diff] [review]
patch

Bouncing to jst; I've no idea if |scx| being null here is a problem or not.

Comment 5

[Justin Dolske [:Dolske]]

See also bug 632833 :)

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 623450 [details] [diff] [review]
patch

Unfortunately I don't see how this could possibly fix anything here. aCallerWin is an nsPIDOMWindow, meaning it's an nsGlobalWindow. nsGlobalWindow directly inherits nsIScriptGlobalObject and a QI call on a valid nsPIDOMWindow pointer to nsIScriptGlobalObject will always succeed, so the null pointer check addition here is not fixing the root cause here. Something went wrong before we got to this point :(

Comment 7

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Also occurs on Native Fennec: https://crash-stats.mozilla.com/report/list?signature=nsGlobalWindow%3A%3ALeaveModalState

Comment 8

[Scoobidiver (away)]

It's #39 top browser crasher in 15.0a2. It first appeared in 15.0a1/20120426. The regression window is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75c7378c87b6&tochange=cc5254f9825f

According to comments, it's related to email spell checking.

Comment 9

[Scoobidiver (away)]

It's #36 top browser crasher and #4 on Mac OS X in 15.0b5.

Comment 10

[Scoobidiver (away)]

It's #14 top browser crasher in 15.0 and #2 on Mac OS X.

It's still related to the spell checker.

Comment 11

[Alex Keybl [:akeybl]]

Adding qawanted, steps-wanted, and needURLs so that we can try to reproduce.

Comment 12

[Scoobidiver (away)]

(In reply to Alex Keybl [:akeybl] from comment #11)
> Adding qawanted, steps-wanted, and needURLs so that we can try to reproduce.
There's already a testcase!

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 623450 [details] [diff] [review]
patch

I believe this is the right fix.
nsIDOMWindow is not a builtinclass, so we may end up calling
LeaveModalStateWithWindow with random JSObject. And if I've
understood correctly how hueyfix works, after closing the window, pointers
to it are transplanted to point to some other JSObject than the original
DOMWindow wrapper.

Per GDB aCallerWin doesn't look like a dead object, and QI certainly fails,
so I don't know what other could cause this problem.

Comment 14

[Alex Keybl [:akeybl]]

(In reply to Scoobidiver from comment #12)
> (In reply to Alex Keybl [:akeybl] from comment #11)
> > Adding qawanted, steps-wanted, and needURLs so that we can try to reproduce.
> There's already a testcase!

Woops

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/bcc2d5b5b351

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 623450 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): hueyfix?
User impact if declined: crashes
Testing completed (on m-c, etc.): just landed m-c. Simple null check 
Risk to taking this patch (and alternatives if risky): should be very low risk
String or UUID changes made by this patch: NA

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 623450 [details] [diff] [review]
patch

[Triage Comment]
Null check for a top crash - let's get this into Aurora 17 and Beta 16.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/440aa0d7c3d3
https://hg.mozilla.org/releases/mozilla-beta/rev/b1ec5d08e466

Comment 20

[Paul Silaghi, QA [:pauly]]

Able to see the issue loading the testcase on Nightly 2012-07-19.
Verified fixed on FF 16b3 on Win 7 x64, Ubuntu 12.04 and Mac OS X 10.6.8

Comment 21

[Paul Silaghi, QA [:pauly]]

Verified fixed on FF 17b2 on Win 7 x64, Ubuntu 12.04 and Mac OS X 10.7.5