Closed
Bug 854082
Opened 12 years ago
Closed 12 years ago
crash in nsPluginInstanceOwner::CreateWidget @ nsObjectFrame::PrepForDrawing
Categories
(Core Graveyard :: Plug-ins, defect, P2)
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)
2.22 KB,
patch
|
benjamin
:
review+
akeybl
:
approval-mozilla-aurora+
johns
:
checkin+
|
Details | Diff | Splinter Review |
944 bytes,
patch
|
benjamin
:
review+
akeybl
:
approval-mozilla-aurora+
johns
:
checkin+
|
Details | Diff | Splinter Review |
7.43 KB,
patch
|
johns
:
review+
johns
:
checkin+
|
Details | Diff | Splinter Review |
3.25 KB,
patch
|
gfritzsche
:
review+
akeybl
:
approval-mozilla-aurora+
johns
:
checkin+
|
Details | Diff | Splinter Review |
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 | ||
Updated•12 years ago
|
Assignee: nobody → jschoenick
Status: NEW → ASSIGNED
Reporter | ||
Updated•12 years ago
|
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…
Updated•12 years ago
|
tracking-firefox22:
--- → +
Priority: -- → P2
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
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?
Attachment #729312 -
Flags: review?(benjamin)
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/
Reporter | ||
Updated•12 years ago
|
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…
Updated•12 years ago
|
Attachment #729312 -
Flags: review?(benjamin) → review+
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
Keywords: needURLs,
steps-wanted
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.
Attachment #729892 -
Flags: review?(benjamin)
Assignee | ||
Updated•12 years ago
|
Keywords: needURLs,
steps-wanted
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
Attachment #729312 -
Attachment is obsolete: true
Reporter | ||
Updated•12 years ago
|
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 7•12 years ago
|
||
Comment on attachment 729892 [details] [diff] [review]
Test
That's an evil test ;-)
Attachment #729892 -
Flags: review?(benjamin) → review+
Reporter | ||
Updated•12 years ago
|
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…
status-firefox23:
--- → affected
Reporter | ||
Updated•12 years ago
|
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*…
![]() |
||
Comment 9•12 years ago
|
||
Can we please get this landed, and uplifted to Aurora as well (see comment #8)?
Flags: needinfo?(jschoenick)
![]() |
||
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.
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)
Flags: needinfo?(jschoenick)
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.
Attachment #738753 -
Flags: review?
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.
Attachment #738754 -
Flags: review?(benjamin)
Assignee | ||
Updated•12 years ago
|
Attachment #738753 -
Flags: review? → review?(benjamin)
Assignee | ||
Comment 14•12 years ago
|
||
qref fail...
Attachment #738754 -
Attachment is obsolete: true
Attachment #738754 -
Flags: review?(benjamin)
Attachment #738756 -
Flags: review?(benjamin)
Comment 15•12 years ago
|
||
rising back up to #8 on 23 and combined signatures put it into top 10 crasher on 22
Assignee | ||
Comment 16•12 years ago
|
||
Updated•12 years ago
|
Attachment #738753 -
Flags: review?(benjamin) → review+
Updated•12 years ago
|
Attachment #738756 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 17•12 years ago
|
||
Add missing null-check, carrying r+
Attachment #738756 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #740382 -
Flags: review+
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
Attachment #740382 -
Flags: checkin+
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
Attachment #738753 -
Flags: checkin+
Assignee | ||
Comment 20•12 years ago
|
||
Comment on attachment 729892 [details] [diff] [review]
Test
https://hg.mozilla.org/integration/mozilla-inbound/rev/43d1b14e8920
Attachment #729892 -
Flags: checkin+
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
Attachment #738753 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #729892 -
Flags: approval-mozilla-aurora?
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
Attachment #729892 -
Flags: checkin-
Attachment #729892 -
Flags: checkin+
Attachment #729892 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #738753 -
Flags: checkin-
Attachment #738753 -
Flags: checkin+
Attachment #738753 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #740382 -
Flags: checkin+ → checkin-
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.
Attachment #740561 -
Flags: review?(georg.fritzsche)
Assignee | ||
Comment 24•12 years ago
|
||
More thorough try push this time
https://tbpl.mozilla.org/?tree=Try&rev=dbfaf4b211bb
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.
Attachment #740561 -
Flags: review?(georg.fritzsche) → review+
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.
Attachment #740561 -
Flags: checkin+
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
Attachment #740382 -
Flags: checkin- → checkin+
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
Attachment #738753 -
Flags: checkin- → checkin+
Assignee | ||
Comment 29•12 years ago
|
||
Comment on attachment 729892 [details] [diff] [review]
Test
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f1599c9c715
Attachment #729892 -
Flags: checkin- → checkin+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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
Attachment #738753 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #729892 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #740561 -
Flags: approval-mozilla-aurora?
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.
Attachment #729892 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #738753 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #740561 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 33•12 years ago
|
||
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•