Closed Bug 854082 Opened 12 years ago Closed 12 years ago

crash in nsPluginInstanceOwner::CreateWidget @ nsObjectFrame::PrepForDrawing

Categories

(Core Graveyard :: Plug-ins, defect, P2)

22 Branch
All
Windows 7
defect

Tracking

(firefox21 unaffected, firefox22+ fixed, firefox23 fixed)

RESOLVED FIXED
mozilla23
Tracking Status
firefox21 --- unaffected
firefox22 + fixed
firefox23 --- fixed

People

(Reporter: scoobidiver, Assigned: johns)

References

Details

(Keywords: crash, regression, topcrash)

Crash Data

Attachments

(4 files, 3 obsolete files)

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
Attached 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?
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)
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
Depends on: 966786
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: