347742 - (1.8 branch) Crash when closing tab with windows media player with onblur handler that removes plugin object

Status: RESOLVED WORKSFORME

(Core Graveyard :: Plug-ins, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase which crash current branch and trunk builds, although with a different stack.
I'm filing this as the 1.8.1 branch crash, the trunk crash gets a different bug, which I'll file shortly hereafter.

Talkback ID from 1.8.1 branch: TB21859306Z
nsContainerFrame::Destroy  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsContainerFrame.cpp, line 147]
nsObjectFrame::Destroy  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsObjectFrame.cpp, line 775]
nsBlockFrame::DoRemoveFrame  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 5782]
nsBlockFrame::RemoveFrame  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsBlockFrame.cpp, line 5567]
nsFrameManager::RemoveFrame  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsFrameManager.cpp, line 705]
nsCSSFrameConstructor::ContentRemoved  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 10043]
PresShell::ContentRemoved  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 5570]
nsDocument::ContentRemoved  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsDocument.cpp, line 2441]
nsHTMLDocument::ContentRemoved  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/html/document/src/nsHTMLDocument.cpp, line 1180]
nsGenericElement::RemoveChildAt  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsGenericElement.cpp, line 2907]
nsGenericElement::RemoveChild  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsGenericElement.cpp, line 3656]
XPTC_InvokeByIndex  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102]
XPCWrappedNative::CallMethod  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2169]
XPC_WN_CallMethod  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1450]
js_Invoke  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1350]
js_Interpret  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 4061]
js_Invoke  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1369]
js_InternalInvoke  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1448]
JS_CallFunctionValue  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsapi.c, line 4402]
nsJSContext::CallEventHandler  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1448]
nsJSEventListener::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/events/nsJSEventListener.cpp, line 195]
nsEventListenerManager::HandleEventSubType  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventListenerManager.cpp, line 1655]
nsEventListenerManager::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventListenerManager.cpp, line 1762]
nsGlobalWindow::HandleDOMEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/base/nsGlobalWindow.cpp, line 1655]
nsDocument::HandleDOMEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/base/src/nsDocument.cpp, line 4053]
nsEventStateManager::SendFocusBlur  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventStateManager.cpp, line 4373]
nsEventStateManager::SetContentState  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventStateManager.cpp, line 4080]
nsXULElement::SetFocus  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/xul/content/src/nsXULElement.cpp, line 2826]
nsEventStateManager::PostHandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/events/src/nsEventStateManager.cpp, line 2120]
PresShell::HandleEventInternal  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 6495]
PresShell::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 6259]
nsViewManager::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp, line 2559]
nsViewManager::DispatchEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp, line 2246]
HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/view/src/nsView.cpp, line 174]
nsWindow::DispatchEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 1348]
nsWindow::DispatchMouseEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 6233]
ChildWindow::DispatchMouseEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 6480]
nsWindow::WindowProc  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp, line 1536]
USER32.dll + 0x8734 (0x77d18734)
USER32.dll + 0x8816 (0x77d18816)
USER32.dll + 0x89cd (0x77d189cd)
USER32.dll + 0x8a10 (0x77d18a10)
nsAppShell::Run  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/widget/src/windows/nsAppShell.cpp, line 159]
nsAppStartup::Run  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp, line 152]
main  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/browser/app/nsBrowserApp.cpp, line 61]
kernel32.dll + 0x16d4f (0x7c816d4f)

I'm marking this security sensitive, just in case.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

This is not fixed by the patch in bug 347662 in my 1.8.1 debug build.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Do you get the same stack?

Comment 4

[Mike Schroepfer]

moving to 1.8.1.1 as this looks like we won't get a fix in time for 1.8.1 and this is not a topcrash.

Comment 5

[Daniel Veditz [:dveditz]]

Doesn't seem to crash FF 1.5.0.8, or am I testing wrong?

Comment 6

[Martijn Wargers (dead)]

I crash with a 1.8.0.9pre build, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.9pre) Gecko/20061110 Firefox/1.5.0.9pre
Talkback ID: TB25827729H

Comment 7

[Boris Zbarsky [:bzbarsky]]

This really needs someone with a Windows build env to look at it.  :(

Comment 8

[Jay Patel [:jay]]

Who can look at this?  Any good candidates to assign this to?

Comment 9

[Johnny Stenback (:jst)]

Taking bug.

Comment 10

[Johnny Stenback (:jst)]

I've been trying to reproduce this on WinXP with Windows Media Player 9.00.00.3349, but no luck. Any more clues what might be needed to see this happen?

Comment 11

[Martijn Wargers (dead)]

The video needs to be playing while you close the tab with the video running in it. If the video isn't playing and you close the tab, it isn't crashing for me.
I'm using Windows Media Player 10.00.00.3646.
New talkback ID: TB28031893H (which seems to be the same, still)

Comment 12

[Johnny Stenback (:jst)]

With that I can reproduce this! Looks like this may be related to bug 354102, I'll investigate...

Comment 13

[Johnny Stenback (:jst)]

So what happens here is that we start closing the tab and fire the blur handler. The blur handler ends up removing the plugin DOM element from the tree, and that destroying the nsObjectFrame, but before destroying the frame the frame is removed from the frame tree. During nsObjectFrame::Destroy(), we stop the plugin, and that ends up calling back into mozilla code to process events, which in turn ends up deleting the nsObjectFrame's parent etc. We then crash at a later point when we either try to reference that parent or unwind and end up with dead |this| pointers etc.

I don't know that there is a branch safe fix for this. Cc:ing roc in case he's got some ideas on this.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can we put the plugin stoppage into an event that runs later? I don't really know if we can keep the plugin alive and stop it after its frame is gone.

Alternatively, does UnbindFromTree run before we destroy the frames? If so, could the plugin's element's UnbindFromTree stop the plugin?

Comment 15

[Johnny Stenback (:jst)]

Interestingly enough, UnbindFromTree() runs before notifying the document about the removal when the *root* element is removed, but *after* removal notifications are fired in the case of removing anything other than the root element. So for plugins, UnbindFromTree() runs after the frame is destroyed. All this is for the branches. On the trunk, we consistently call UnbindFromTree() after notifying the document about the removal.

And yeah, stopping the plugin off of an event would solve this, but that's tricky due to the frame going away, and especially if the native plugin widget goes away before the plugin does. Actually, I don't think the frame going away is all that critical, but we need to make sure the widget (native HWND or whatever) doesn't get deleted as a result of the frame tree going away. Is there a way to hold on to a widget (including its parent etc) so that we can ensure that stays alive until we stop the plugin?

The more I think about this the less I want to touch any of this on the brances :(

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The problem with stopping from UnbindFromTree is that we don't want to run scripts there either. There currently are some ways where scripts can run, but we're trying to fix them to get things more stable.

Comment 17

[Daniel Veditz [:dveditz]]

The testcase currently involves pretty specific user interaction, but I bet a page could find a way to force this sequence or one similar enough.

Given comment 15 (scary on the branch) we'd definitely want to understand the trunk fix before attempting to fix this on the branch which will take more study.

Comment 18

[chris hofmann]

sounds like this would be a good one for upcoming arch discussions.  anyone have more comments to prime the pump on those discussions, or more thoughts about approaches to fixing?

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bug 347743 is sort of more interesting since we can do better stuff on trunk. Basically we should figure out a way to set up and tear down the plugin at a point when we're not in the middle of layout. The tricky part is that the plugin requires some objects that are torn down by layout (views iirc) so we'd have to keep them alive past the relayout.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We should rework the plugin code so it doesn't require views.

Comment 21

[Johnny Stenback (:jst)]

Roc, is that something you can work on, or can you lay out what would need to be done to make that happen?

Comment 22

[Johnny Stenback (:jst)]

Though in all honesty, that really doesn't sound like something we would want to do for the branch. For the trunk, for sure though.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, for trunk. The hard part is that plugins with windows need a view to attach the widget to. On trunk I'm already working on a solution to this that doesn't attach the widget to the view (although that work is stalled). For branch, this would be very hard. To be useful you'd have to change the lifetime of the widget so it wasn't torn down when the rest of its widget hierarchy (which depends on frames) was destroyed. Maybe you could reparent the widget somewhere safe, but ugh.

Comment 24

[Daniel Veditz [:dveditz]]

Clearing branch nominations based on the last few comments.

Comment 25

[Martijn Wargers (dead)]

Attachment: testcase

Comment 26

[chris hofmann]

roc, do you think the trunk work make it for 1.9?

Comment 27

[Johnny Stenback (:jst)]

The trunk work for this is tracked in bug 347743. Minusing this one for 1.9.

Comment 28

[Daniel Veditz [:dveditz]]

Given that the trunk version has been fixed now, does that provide any useful hints on how to patch this on branch?

Comment 29

[Johnny Stenback (:jst)]

Yes, the patch is reasonably easily portable to the branches and the surrounding code should be pretty much the same still AFAIK, but before I do that I want to see the trunk version bake for a non-trivial amount of time as there's certainly possibilities for crashes etc hiding there still.

Comment 30

[Daniel Veditz [:dveditz]]

If the fix for bug 347743 ships in 1.9a6 Monday we should get plenty of feedback on whether this is a good idea or not in time for the July 13 branch code-freeze. 

Comment 31

[Johnny Stenback (:jst)]

The fix for bug 347743 was backed out and will not make it into a6. See the bug for details.

Comment 32

[Daniel Veditz [:dveditz]]

removing from blocking list until we get 347743 straightened out

Comment 33

[Martijn Wargers (dead)]

Maybe this should become a wontfix?

Comment 34

[Daniel Veditz [:dveditz]]

bug 347743 got fixed on the trunk a year ago, is this still out of reach for the 1.8 branch?

Comment 35

[Brandon Sterne (:bsterne)]

Since this is 1.8 branch-only and we are approaching EOL for Firefox 2, shall we WONTFIX this?

Comment 36

[Martijn Wargers (dead)]

Fine by me.

Comment 37

[Martijn Wargers (dead)]

Oops, I forgot I'm not allowed to wontfix. Probably jst or bz should resolve it to wontfix.

Comment 38

[Martijn Wargers (dead)]

Marking resolved worksforme, as this is only a problem in the 1.8 branch and not in later builds and the 1.8 branch is not maintained anymore by Mozilla.