crash in nsPluginInstanceOwner::CreateWidget @ nsObjectFrame::PrepForDrawing

RESOLVED FIXED in Firefox 22

Status

()

defect
P2
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: scoobidiver, Assigned: johns)

Tracking

({crash, regression, topcrash})

22 Branch
mozilla23
All
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 unaffected, firefox22+ fixed, firefox23 fixed)

Details

(crash signature)

Attachments

(4 attachments, 3 obsolete attachments)

It first showed up in 22.0a1/20130321090706. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8156df33b757&tochange=a73a2b5c423b (worst case)
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1d6fe70c79c5&tochange=a73a2b5c423b (best case)

Signature 	nsCOMPtr_base::assign_with_AddRef(nsISupports*) | nsCOMPtr<nsIWidget>::operator=(nsIWidget*) | nsObjectFrame::PrepForDrawing(nsIWidget*) More Reports Search
UUID	8e02d87f-d88a-41bb-b1a7-4eaaa2130322
Date Processed	2013-03-22 22:16:49
Uptime	502
Last Crash	8.4 minutes before submission
Install Age	6.1 hours since version was first installed.
Install Time	2013-03-22 16:09:09
Product	Firefox
Version	22.0a1
Build ID	20130322031028
Release Channel	nightly
OS	Windows NT
OS Version	5.1.2600 Dodatek Service Pack 3
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 23 stepping 10
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0xfffffffff0de7fff
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x0640, AdapterSubsysID: 00000000, AdapterDriverVersion: 6.14.11.9107
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers+ 
Processor Notes 	sp-processor10.phx1.mozilla.com_29273:2008
EMCheckCompatibility	True
Adapter Vendor ID	0x10de
Adapter Device ID	0x0640
Total Virtual Memory	2147352576
Available Virtual Memory	1666146304
System Memory Use Percentage	46
Available Page File	3332423680
Available Physical Memory	1138520064

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsCOMPtr_base::assign_with_AddRef 	obj-firefox/xpcom/build/nsCOMPtr.cpp:49
1 	xul.dll 	nsCOMPtr<nsIWidget>::operator= 	obj-firefox/dist/include/nsCOMPtr.h:624
2 	xul.dll 	nsObjectFrame::PrepForDrawing 	layout/generic/nsObjectFrame.cpp:356
3 	xul.dll 	nsPluginInstanceOwner::CreateWidget 	dom/plugins/base/nsPluginInstanceOwner.cpp:3112
4 	xul.dll 	nsPluginHost::InstantiatePluginInstance 	dom/plugins/base/nsPluginHost.cpp:955
5 	xul.dll 	nsObjectLoadingContent::InstantiatePluginInstance 	content/base/src/nsObjectLoadingContent.cpp:765
6 	xul.dll 	nsObjectLoadingContent::LoadObject 	content/base/src/nsObjectLoadingContent.cpp:1992
7 	xul.dll 	nsObjectLoadingContent::OnStartRequest 	content/base/src/nsObjectLoadingContent.cpp:893
8 	xul.dll 	mozilla::net::nsHttpChannel::CallOnStartRequest 	netwerk/protocol/http/nsHttpChannel.cpp:959
9 	xul.dll 	mozilla::net::nsHttpChannel::ContinueOnStartRequest2 	netwerk/protocol/http/nsHttpChannel.cpp:4938
10 	xul.dll 	mozilla::net::nsHttpChannel::OnStartRequest 	netwerk/protocol/http/nsHttpChannel.cpp:4911
11 	xul.dll 	nsInputStreamPump::OnStateStart 	netwerk/base/src/nsInputStreamPump.cpp:418
12 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:369
13 	xul.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:82
14 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
15 	xul.dll 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:238
16 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:82
17 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:209
18 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:183
19 	xul.dll 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:163
20 	xul.dll 	nsAppShell::Run 	widget/windows/nsAppShell.cpp:161
21 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:288
22 	xul.dll 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3880
23 	xul.dll 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3947
24 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4161
25 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp:228
26 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:105
27 	firefox.exe 	__tmainCRTStartup 	crtexe.c:552
28 	kernel32.dll 	BaseProcessStart 	

More reports at:
https://crash-stats.mozilla.com/query/query?product=Firefox&version=Firefox%3A22.0a1&query_search=signature&query_type=contains&query=nsObjectFrame%3A%3APrepForDrawing&do_query=1
Assignee: nobody → jschoenick
Status: NEW → ASSIGNED
Crash Signature: nsObjectFrame::PrepForDrawing(nsIWidget*) ] → nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsCOMPtr_base::assign_with_AddRef(nsISupports*) | nsCOMPtr<nsIRDFResource>::operator=(nsIRDFResource*) | nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsCOMPtr_base::assign_with_AddRef(nsISupports*) | nsC…
Priority: -- → P2
Blocks: 843671
Actually 843671 didn't have the instantiation change I'm thinking of, this is most likely from bug 784131
Blocks: 784131
No longer blocks: 843671
So I can't reproduce this, but reading through the codepath here reveals that
RunInStableState is not what we want to use in HasNewFrame, as it may decide to
run the event synchronously, which I think could lead to the witnessed behavior.

The downside of not using it is that script that causes an addon to lose its
frame but then spins a nested event loop (e.g. by calling into plugin code)
could cause the plugin to despawn before exiting the current event. I'm not sure
what the spec says about this -- Are nested event loops inside JS are allowed in
the first place?

@bsmedberg is there a sane way around the latter occurance?
Attachment #729312 - Flags: review?(benjamin)
(In reply to John Schoenick [:johns] from comment #2)
> ... downside of not using it is that script that causes an addon to lose ...

s/addon/plugin/
Crash Signature: nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) | nsCOMPtr<nsIWebProgressListener>::operator=(nsIWebProgressListener*) | nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ @0x0 | nsObjectFrame::PrepForDraw… → nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) | nsCOMPtr<nsIWebProgressListener>::operator=(nsIWebProgressListener*) | nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsCOMPtr_base::assign_with_AddRef…
Attachment #729312 - Flags: review?(benjamin) → review+
I talked to bsmedberg on IRC and I think this patch is good, but we need to find STR to make sure that nothing else odd is going on
Posted patch TestSplinter Review
Okay, so, turns out that RunInStableState re-entry is a red herring here. I managed to un-tangle what's going on here: InstanceOwner looks at its content node and fetches a frame in ::Init(), so it has a frame before content has a pointer to mInstanceOwner, which means if the frame goes away content cannot forward the SetFrame(null) call along. This test reproduces the crash -- Patch incoming that untangles the frame ownership, I'll spin off another bug for the StableState stuff.
Attachment #729892 - Flags: review?(benjamin)
Comment on attachment 729312 [details] [diff] [review]
Don't use RunInStableState for queuing CheckPluginStopEvent from HasNewFrame

bug 855139 filed for RunInStableState
Attachment #729312 - Attachment is obsolete: true
Crash Signature: nsObjectFrame::PrepForDrawing(nsIWidget*) ] → nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsCOMPtr_base::assign_with_AddRef(nsISupports*) | nsCOMPtr<nsIStreamListener>::operator=(nsIStreamListener*) | nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsCOMPtr_base::assign_with_AddRef(nsISupports*)…
Comment on attachment 729892 [details] [diff] [review]
Test

That's an evil test ;-)
Attachment #729892 - Flags: review?(benjamin) → review+
See Also: → 858773
Crash Signature: nsObjectFrame::PrepForDrawing(nsIWidget*) ] → nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsCOMPtr_base::assign_with_AddRef(nsISupports*) | nsCOMPtr<mozilla::dom::Element>::operator=(mozilla::dom::Element*) | nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsCOMPtr_base::assign_with_AddRef(nsISu…
It's #9 top browser crasher in 22.0a2.
Keywords: topcrash
Crash Signature: nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsCOMPtr_base::assign_with_AddRef(nsISupports*) | nsCOMPtr<mozilla::dom::Element>::operator=(mozilla::dom::Element*) | nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsObjectFrame::PrepForDrawing(nsIWidget… → nsObjectFrame::PrepForDrawing(nsIWidget*) ] [@ nsCOMPtr_base::assign_with_AddRef(nsISupports*) | nsCOMPtr<mozilla::dom::Element>::operator=(mozilla::dom::Element*) | nsObjectFrame::PrepForDrawing(nsIWidget*)] [@ nsObjectFrame::PrepForDrawing(nsIWidget*…
Can we please get this landed, and uplifted to Aurora as well (see comment #8)?
Flags: needinfo?(jschoenick)
Oh, I see, this is just a test. We'd need a fix, actually. Still, can we have some progress on that? This continues to be a topcrash on 22 and 23.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #10)
> Oh, I see, this is just a test. We'd need a fix, actually. Still, can we
> have some progress on that? This continues to be a topcrash on 22 and 23.

This is next on the list after the beta topcrash (bug 859099)
Flags: needinfo?(jschoenick)
So I removed a seemingly-redundant SetFrame(null) call in bug 784131, but it
turns out it was covering for other ownership confusion when frames are
destroyed during re-entry. This patch just re-adds that call.
Attachment #738753 - Flags: review?
The previous patch fixes the crash and is safe for beta, but the scenario that
causes it would result in a plugin unaware of its frame, causing events/painting
to possibly fail.

This cleans up the ownership model and adds more explicit comments around the
remaining less-than-optimal spots.
Attachment #738754 - Flags: review?(benjamin)
Attachment #738753 - Flags: review? → review?(benjamin)
qref fail...
Attachment #738754 - Attachment is obsolete: true
Attachment #738754 - Flags: review?(benjamin)
Attachment #738756 - Flags: review?(benjamin)
rising back up to #8 on 23 and combined signatures put it into top 10 crasher on 22
Attachment #738753 - Flags: review?(benjamin) → review+
Attachment #738756 - Flags: review?(benjamin) → review+
Add missing null-check, carrying r+
Attachment #738756 - Attachment is obsolete: true
Attachment #740382 - Flags: review+
Comment on attachment 740382 [details] [diff] [review]
Cleanup plugin frame ownership, prevent losing our frame due to re-entrance. r=bsmedberg

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3eaea876a18
Attachment #740382 - Flags: checkin+
Comment on attachment 738753 [details] [diff] [review]
Restore SetFrame(null) call to avoid instance owners pointing to dead frames

https://hg.mozilla.org/integration/mozilla-inbound/rev/3aaf738a04d8
Attachment #738753 - Flags: checkin+
Comment on attachment 738753 [details] [diff] [review]
Restore SetFrame(null) call to avoid instance owners pointing to dead frames

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

User impact if declined: 
Crash

Testing completed (on m-c, etc.):
Soon-to-be-on m-c

Risk to taking this patch (and alternatives if risky):
Low, reverts small code change from 784131. Alternative would be full backout of 784131 from aurora. (The larger patch isn't necessary for fixing the crash and is only landing on trunk)

String or IDL/UUID changes made by this patch:
None
Attachment #738753 - Flags: approval-mozilla-aurora?
Attachment #729892 - Flags: approval-mozilla-aurora?
Comment on attachment 729892 [details] [diff] [review]
Test

Bounced for assertion failures on OS X :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/58011469a3c9
Attachment #729892 - Flags: checkin-
Attachment #729892 - Flags: checkin+
Attachment #729892 - Flags: approval-mozilla-aurora?
Attachment #738753 - Flags: checkin-
Attachment #738753 - Flags: checkin+
Attachment #738753 - Flags: approval-mozilla-aurora?
Attachment #740382 - Flags: checkin+ → checkin-
Blocks: 863792
The reason for the failures was that bug 813906's test was triggering assertions that were being attributed to a few random tests after it. This makes sure said test hits the assertion before finishing, and moves the assertion annotations there. The assertion itself is bug 621618.
Attachment #740561 - Flags: review?(georg.fritzsche)
More thorough try push this time

https://tbpl.mozilla.org/?tree=Try&rev=dbfaf4b211bb
Comment on attachment 740561 [details] [diff] [review]
Attribute bug 621618 assertions to the proper test

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

This looks good to me. Please note that i'm not sure in which cases i actually currently can review.

::: dom/plugins/test/mochitest/test_bug813906.html
@@ +28,5 @@
>  <script type="application/javascript">
>  SimpleTest.waitForExplicitFinish();
>  
> +// When the document is torn down or <svg> is removed, we hit bug 621618
> +SimpleTest.expectAssertions(1);

Please make this Mac-only too to not accidentally cover assertions on other platforms.
Attachment #740561 - Flags: review?(georg.fritzsche) → review+
Comment on attachment 740561 [details] [diff] [review]
Attribute bug 621618 assertions to the proper test

Okay, let's try this again. Relevant try run: https://tbpl.mozilla.org/?tree=Try&rev=dbfaf4b211bb

https://hg.mozilla.org/integration/mozilla-inbound/rev/e03057b8c8ca

(In reply to Georg Fritzsche [:gfritzsche] from comment #25)
> Please make this Mac-only too to not accidentally cover assertions on other
> platforms.

This actually happens everywhere, it was only mac-only for this test because it would be annotated to test_cookies on other platforms.
Attachment #740561 - Flags: checkin+
Comment on attachment 740382 [details] [diff] [review]
Cleanup plugin frame ownership, prevent losing our frame due to re-entrance. r=bsmedberg

https://hg.mozilla.org/integration/mozilla-inbound/rev/9844a2fe0f46
Attachment #740382 - Flags: checkin- → checkin+
Comment on attachment 738753 [details] [diff] [review]
Restore SetFrame(null) call to avoid instance owners pointing to dead frames

https://hg.mozilla.org/integration/mozilla-inbound/rev/2db093c411a5
Attachment #738753 - Flags: checkin- → checkin+
Comment on attachment 738753 [details] [diff] [review]
Restore SetFrame(null) call to avoid instance owners pointing to dead frames

Okay let's try this again

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

User impact if declined: 
Crash

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

Risk to taking this patch (and alternatives if risky):
Low, reverts small code change from 784131. Alternative would be full backout of 784131 from aurora. (The larger patch isn't necessary for fixing the crash and is only landing on trunk)

String or IDL/UUID changes made by this patch:
None
Attachment #738753 - Flags: approval-mozilla-aurora?
Attachment #729892 - Flags: approval-mozilla-aurora?
Attachment #740561 - Flags: approval-mozilla-aurora?
Comment on attachment 729892 [details] [diff] [review]
Test

Blocks bug 863792, which will hopefully resolve bug 859099. Rushing for Aurora 22 in preparation for possible landing to Beta 21.
Attachment #729892 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #738753 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #740561 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 861140
Duplicate of this bug: 858773
Duplicate of this bug: 861140
Depends on: 966786
You need to log in before you can comment on or make changes to this bug.