871942 - crash in nsWebShellWindow::RequestWindowClose

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Scoobidiver (away)]

It's #87 browser crasher in 21.0b7, #143 in 22.0a2, and #289 in 23.0a1.
It first showed up in 21.0a1/20130109. The regression range might be (discontinuous across builds):
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dccab70d8554&tochange=7bce868864bf
It might be a regression from bug 827188.

Comments talk about Flash or Java.

Signature 	nsWebShellWindow::RequestWindowClose(nsIWidget*) More Reports Search
UUID	edcf9f96-7cb0-414b-8335-6e8662130510
Date Processed	2013-05-10 09:37:01
Uptime	2744
Last Crash	5.7 days before submission
Install Age	16.1 hours since version was first installed.
Install Time	2013-05-09 17:33:11
Product	Firefox
Version	23.0a1
Build ID	20130509031047
Release Channel	nightly
OS	Windows NT
OS Version	6.2.9200
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 23 stepping 10
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x8
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x9553, AdapterSubsysID: 00000000, AdapterDriverVersion: 8.970.100.7000
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ D3D10 Layers- D3D9 Layers? D3D9 Layers+ 
Processor Notes 	sp-processor07_phx1_mozilla_com_16448:2012; MDSW emitted too many frames, triggering truncation
EMCheckCompatibility	True
Adapter Vendor ID	0x1002
Adapter Device ID	0x9553
Total Virtual Memory	2147352576
Available Virtual Memory	1159766016
System Memory Use Percentage	68
Available Page File	2557849600
Available Physical Memory	1008316416

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsWebShellWindow::RequestWindowClose 	xpfe/appshell/src/nsWebShellWindow.cpp:302
1 	xul.dll 	nsWindow::ProcessMessage 	widget/windows/nsWindow.cpp:4766
2 	xul.dll 	nsWindow::WindowProcInternal 	widget/windows/nsWindow.cpp:4388
3 	xul.dll 	CallWindowProcCrashProtected 	xpcom/base/nsCrashOnException.cpp:32
4 	xul.dll 	nsWindow::WindowProc 	widget/windows/nsWindow.cpp:4340
5 	user32.dll 	InternalCallWinProc 	
6 	user32.dll 	UserCallWinProcCheckWow 	
7 	user32.dll 	DispatchClientMessage 	
8 	ntdll.dll 	KiUserCallbackDispatcher 	
9 	xul.dll 	xul.dll@0xb5630 	
10 	user32.dll 	RealDefWindowProcW 	
11 	uxtheme.dll 	uxtheme.dll@0x2d710 	
12 	uxtheme.dll 	uxtheme.dll@0x2e27 	
13 	uxtheme.dll 	uxtheme.dll@0x2a39 	
14 	user32.dll 	GetRealWindowOwner 	
15 	user32.dll 	InternalCallWinProc 	
16 	user32.dll 	UserCallWinProcCheckWow 	
17 	user32.dll 	CallWindowProcW 	
18 	xul.dll 	nsWindow::WindowProcInternal 	widget/windows/nsWindow.cpp:4393
19 	xul.dll 	CallWindowProcCrashProtected 	xpcom/base/nsCrashOnException.cpp:32
20 	xul.dll 	nsWindow::WindowProc 	widget/windows/nsWindow.cpp:4340
21 	user32.dll 	InternalCallWinProc 	
22 	user32.dll 	UserCallWinProcCheckWow 	
23 	user32.dll 	CallWindowProcW 	
24 	xul.dll 	mozilla::ipc::windows::DeferredSendMessage::Run 	ipc/glue/WindowsMessageLoop.cpp:987
25 	xul.dll 	`anonymous namespace'::DeferredMessageHook 	ipc/glue/WindowsMessageLoop.cpp:132
26 	user32.dll 	fnHkINLPCWPSTRUCTW 	
27 	user32.dll 	__fnDWORD 	
28 	ntdll.dll 	KiUserCallbackDispatcher 	
29 	ntdll.dll 	KiUserApcDispatcher 	
30 	xul.dll 	nsPluginInstanceOwner::Destroy 	dom/plugins/base/nsPluginInstanceOwner.cpp:2560
31 	xul.dll 	nsObjectLoadingContent::DoStopPlugin 	content/base/src/nsObjectLoadingContent.cpp:2578
32 	xul.dll 	nsObjectLoadingContent::StopPluginInstance 	content/base/src/nsObjectLoadingContent.cpp:2627
33 	xul.dll 	nsObjectLoadingContent::DestroyContent 	content/base/src/nsObjectLoadingContent.cpp:2143
34 	xul.dll 	mozilla::dom::HTMLObjectElement::DestroyContent 	content/html/content/src/HTMLObjectElement.cpp:438
35 	xul.dll 	mozilla::dom::FragmentOrElement::DestroyContent 	content/base/src/FragmentOrElement.cpp:952
36 	xul.dll 	nsDocumentViewer::Destroy 	layout/base/nsDocumentViewer.cpp:1618
37 	xul.dll 	nsDocShell::Destroy 	docshell/base/nsDocShell.cpp:4901
38 	xul.dll 	nsFrameLoader::Finalize 	content/base/src/nsFrameLoader.cpp:564
39 	xul.dll 	nsDocument::MaybeInitializeFinalizeFrameLoaders 	content/base/src/nsDocument.cpp:6190
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsWebShellWindow%3A%3ARequestWindowClose%28nsIWidget*%29

Comment 1

[John Schoenick [:johns]]

Hey look, we're re-entering. In plugin code!

That's never happened before.

Comment 2

[Timothy Nikkel (:tnikkel)]

I thought you would enjoy it. :)

Comment 3

[John Schoenick [:johns]]

Attachment: Check for closing-window reentrance in nsWebShellWindow::RequestWindowClose

Null-check the presShell here, and assert that the only situation in which that should occur is when the window is closing

Comment 4

[John Schoenick [:johns]]

Attachment: Don't synchronously stop plugins in DestroyContent

I see no reason to stop plugins synchronously here, other than providing a vector to hit re-entrance bugs.

Comment 5

[John Schoenick [:johns]]

Try push because plugins:
https://tbpl.mozilla.org/?tree=Try&rev=adafb5e4691d

Comment 6

[John Schoenick [:johns]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9953a7663df4
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fab16a30ed5

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9953a7663df4
https://hg.mozilla.org/mozilla-central/rev/1fab16a30ed5

Comment 8

[John Schoenick [:johns]]

Comment on attachment 750803 [details] [diff] [review]
Check for closing-window reentrance in nsWebShellWindow::RequestWindowClose

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Long existing bug only recently being hit

User impact if declined:
Crash

Testing completed (on m-c, etc.): 
On m-c

Risk to taking this patch (and alternatives if risky): 
Low, missing null check. (The other patch, which touches plugins, needn't be uplifted)

String or IDL/UUID changes made by this patch:
None

Comment 9

[John Schoenick [:johns]]

Comment on attachment 750803 [details] [diff] [review]
Check for closing-window reentrance in nsWebShellWindow::RequestWindowClose

Could be a ride-along if we do a point release. #112 top crash on 21, very low risk null check.

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 750803 [details] [diff] [review]
Check for closing-window reentrance in nsWebShellWindow::RequestWindowClose

No planned FF21 dot releases, approving for Aurora/Beta given the very low risk evaluation and where we are in the cycle.

Comment 11

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8362808c60c8

Comment 12

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d8ccf3ca4bd

Comment 13

[Timothy Nikkel (:tnikkel)]

Should we uplift the other patch to prevent that re-entrancy from causing any other problems?

Comment 14

[John Schoenick [:johns]]

(In reply to Timothy Nikkel (:tn) from comment #13)
> Should we uplift the other patch to prevent that re-entrancy from causing
> any other problems?

I talked with jst about this, and he believes we need to be re-entrant in this situation, so the actual bug is just the missing null-check. Given the number of plugin-related regressions we've had in the last few cycles, it's probably better to not uplift more than strictly necessary.

Comment 15

[Manuela Muntean [Away]]

There are no reports in Socorro for the last 4 weeks, regarding this signature.

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 750804 [details] [diff] [review]
Don't synchronously stop plugins in DestroyContent

There is pretty strong evidence that this fixed bug 850571.

Given that this has had lots of bake time now, are you confident enough in uplifting this to beta now? Or should we stick with letting it ride the trains?

Comment 17

[John Schoenick [:johns]]

Comment on attachment 750804 [details] [diff] [review]
Don't synchronously stop plugins in DestroyContent

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Unknown / Long standing

User impact if declined:
Per comment #16, this fixed (another) topcrash in bug 850571

Testing completed (on m-c, etc.): 
On m-c and now m-a without any reported regressions

Risk to taking this patch (and alternatives if risky): 
Low, one-line patch to stop plugins asynchronously (known to be safe) in a spot where they were needlessly stopping synchronously (known to be a re-entrance hazard).

String or IDL/UUID changes made by this patch:
None

Comment 18

[Ioana (away)]

There is only one Firefox 23 crash with this signature in the last 4 weeks (31st of May):
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=nsWebShellWindow%3A%3ARequestWindowClose%28nsIWidget%2A%29

Marking the bug as verified considering that that crash doesn't have the same stack trace and it's a singular crash.

Comment 19

[Timothy Nikkel (:tnikkel)]

I would check that the crashes from bug 850571 get fixed, since that was the impetus for uplifting this to 23.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 750804 [details] [diff] [review]
Don't synchronously stop plugins in DestroyContent

approving uplift, will check back to see if it does fix bug 850571 on FF23

Comment 21

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6630322716e0