Scoobidiver (away) Reporter
	Description • 12 years ago

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

	John Schoenick [:johns] Assignee
	Comment 1 • 12 years ago

Actually 843671 didn't have the instantiation change I'm thinking of, this is most likely from bug 784131

	John Schoenick [:johns] Assignee
	Comment 2 • 12 years ago

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?

	John Schoenick [:johns] Assignee
	Comment 3 • 12 years ago

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

	John Schoenick [:johns] Assignee
	Comment 4 • 12 years ago

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

	John Schoenick [:johns] Assignee
	Comment 5 • 12 years ago

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.

	John Schoenick [:johns] Assignee
	Comment 6 • 12 years ago

Comment on attachment 729312 [details] [diff] [review]
Don't use RunInStableState for queuing CheckPluginStopEvent from HasNewFrame

bug 855139 filed for RunInStableState

	Benjamin Smedberg
	Comment 7 • 12 years ago

Comment on attachment 729892 [details] [diff] [review]
Test

That's an evil test ;-)

	Scoobidiver (away) Reporter
	Comment 8 • 12 years ago

It's #9 top browser crasher in 22.0a2.

	Robert Kaiser
	Comment 9 • 12 years ago

Can we please get this landed, and uplifted to Aurora as well (see comment #8)?

	Robert Kaiser
	Comment 10 • 12 years ago

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.

	John Schoenick [:johns] Assignee
	Comment 11 • 12 years ago

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

	John Schoenick [:johns] Assignee
	Comment 12 • 12 years ago

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.

	John Schoenick [:johns] Assignee
	Comment 13 • 12 years ago

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.

	John Schoenick [:johns] Assignee
	Comment 14 • 12 years ago

qref fail...

	Tracy Walker [:tracy]
	Comment 15 • 12 years ago

rising back up to #8 on 23 and combined signatures put it into top 10 crasher on 22

	John Schoenick [:johns] Assignee
	Comment 16 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=2d141ef6af87

	John Schoenick [:johns] Assignee
	Comment 17 • 12 years ago

Add missing null-check, carrying r+

	John Schoenick [:johns] Assignee
	Comment 18 • 12 years ago

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

	John Schoenick [:johns] Assignee
	Comment 19 • 12 years ago

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

	John Schoenick [:johns] Assignee
	Comment 20 • 12 years ago

Comment on attachment 729892 [details] [diff] [review]
Test

https://hg.mozilla.org/integration/mozilla-inbound/rev/43d1b14e8920

	John Schoenick [:johns] Assignee
	Comment 21 • 12 years ago

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

	John Schoenick [:johns] Assignee
	Comment 22 • 12 years ago

Comment on attachment 729892 [details] [diff] [review]
Test

Bounced for assertion failures on OS X :(

https://hg.mozilla.org/integration/mozilla-inbound/rev/58011469a3c9

	John Schoenick [:johns] Assignee
	Comment 23 • 12 years ago

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.

	John Schoenick [:johns] Assignee
	Comment 24 • 12 years ago

More thorough try push this time

https://tbpl.mozilla.org/?tree=Try&rev=dbfaf4b211bb

	Georg Fritzsche [:gfritzsche]
	Comment 25 • 12 years ago

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.

	John Schoenick [:johns] Assignee
	Comment 26 • 12 years ago

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.

	John Schoenick [:johns] Assignee
	Comment 27 • 12 years ago

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

	John Schoenick [:johns] Assignee
	Comment 28 • 12 years ago

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

	John Schoenick [:johns] Assignee
	Comment 29 • 12 years ago

Comment on attachment 729892 [details] [diff] [review]
Test

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f1599c9c715

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/e03057b8c8ca
https://hg.mozilla.org/mozilla-central/rev/8f1599c9c715
https://hg.mozilla.org/mozilla-central/rev/2db093c411a5
https://hg.mozilla.org/mozilla-central/rev/9844a2fe0f46

	John Schoenick [:johns] Assignee
	Comment 31 • 12 years ago

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

	Alex Keybl [:akeybl]
	Comment 32 • 12 years ago

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.

	John Schoenick [:johns] Assignee
	Comment 33 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/92d57584f6fa
https://hg.mozilla.org/releases/mozilla-aurora/rev/ee0f68deb1f7
https://hg.mozilla.org/releases/mozilla-aurora/rev/3c8896003c08