416953 - Crash quitting while Flash is playing (re-entering frame destruction?)

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: crash stack trace

I hit Cmd+Q while I had several MyFreeCams windows open and Firefox crashed, badly.  I haven't tried to reproduce.  The stack trace might reveal the bug: looks like Flash is somehow causing frame destruction to be re-entered.

###!!! ASSERTION: Stop called too early or too late: 'mDocument', file /Users/jruderman/trunk/mozilla/layout/base/nsDocumentViewer.cpp, line 1539

###!!! ASSERTION: No document in Destroy()!: 'mDocument', file /Users/jruderman/trunk/mozilla/layout/base/nsDocumentViewer.cpp, line 1369


Exception:  EXC_BAD_INSTRUCTION (0x0002)
Code[0]:    0x00000001
Code[1]:    0x00000000

Thread 0 Crashed:
0   <<00000000>> 	0x4667c1b3 0 + 1181204915
1   libgklayout.dylib              	0x17e99793 nsFrameManager::CaptureFrameStateFor(nsIFrame*, nsILayoutHistoryState*, nsIStatefulFrame::SpecialStateID) + 99 (nsFrameManager.cpp:1536)
2   libgklayout.dylib              	0x17e99955 nsFrameManager::CaptureFrameState(nsIFrame*, nsILayoutHistoryState*) + 107 (nsFrameManager.cpp:1574)
...

Comment 1

[Bob Clary [:bc] (inactive)]

different crash altogether in linux/flash9 r115:

firefox-bin: pcm_params.c:2351: sndrv_pcm_hw_params: Assertion `err >= 0' failed.

Program /work/mozilla/builds/1.9.0/mozilla/firefox-debug/dist/bin/firefox-bin (pid = 7295) received signal 6.
Stack:
UNKNOWN 0x110420
abort+0x00000101 [/lib/libc.so.6 +0x0002AF91]
__assert_fail+0x000000EE [/lib/libc.so.6 +0x0002293E]
UNKNOWN [/lib/libasound.so.2 +0x000527EA]
snd_pcm_hw_params+0x00000031 [/lib/libasound.so.2 +0x0004D9C1]
flash stuff...

Comment 2

[Michelle Sintov]

Bob, please file a new bug on your crash, along with specific steps to reproduce. Thanks.

Comment 3

[Bob Clary [:bc] (inactive)]

filed Bug 417127

Comment 4

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

Confirming based on stack.

Jst: I thought that we had fixed plugin teardown by reparenting the window or some such? Looks like we're still shutting down synchronously though.

Jesse: Shouldn't this be sg:critical?

Comment 5

[Jesse Ruderman]

This is sg:critical assuming web sites can make it fairly likely to happen by using Flash in a certain way.  It's not hard to encourage a user to close a window or quit Firefox.

Comment 6

[Jesse Ruderman]

A very similar crash in Safari: http://bugs.webkit.org/show_bug.cgi?id=17466

This might be a bug in Flash or in the Mac's "Sound Manager".

Comment 7

[Johnny Stenback (:jst)]

(In reply to comment #4)
> Confirming based on stack.
> 
> Jst: I thought that we had fixed plugin teardown by reparenting the window or
> some such? Looks like we're still shutting down synchronously though.

We do, but only for Win32 :(

Come to think of it, we could actually extend the delayed plugin destruction scheme to other platforms as well, at least for the plugins that don't claim they need destroy() before they get a setwindow(null) call (and in reality that's all NPAPI plugins), which is what requires the reparenting and delayed destruction of the plugin widget. We could simply do the setwindow(null) call when the frame goes away, and then do the stop() and destroy() calls off of a runnable. That would likely fix this bug...

Comment 8

[Johnny Stenback (:jst)]

Attachment: Possible fix.

Jesse, or anyone else, could you give this a try? This is totally untested, but does compile here.

Comment 9

[Johnny Stenback (:jst)]

Builds with the attached fix (merged to trunk) available here:

https://build.mozilla.org/tryserver-builds/2008-03-08_23:[e-mail]-flash-crash/

Jesse, or anyone with a mac, please give these builds a spin and try to reproduce this if possible.

Comment 10

[Jesse Ruderman]

Without the patch, I can crash reliably with these steps:
1. Load http://www.myfreecams.com/ (must be logged in)
2. Click 4 "Popup" links.
3. Wait for the streaming videos in all those windows to start.
4. With one of the popup windows focused, press Cmd+Q.
Result: Crash [@ nsFrameManager::CaptureFrameStateFor] calling a random address.

With the patch, the same steps leads to a different crash: [@ nsPluginInstanceOwner::FixUpPluginWindow] dereferencing 0x00000024.  This crash seems to happen earlier, and audio (but not video) continues while the system crash reporter gathers a stack trace.  In fact, merely closing a myfreecams.com popup window with Cmd+W can trigger this crash.

Tested with Mac trunk debug builds.

Comment 11

[Jesse Ruderman]

Attachment: stack trace for the nsPluginInstanceOwner::FixUpPluginWindow crash

Comment 12

[Jesse Ruderman]

Btw, I merged the last hunk of your patch to trunk as

-    DoStopPlugin(owner);
+    DoStopPlugin(owner, PR_FALSE);

Comment 13

[Johnny Stenback (:jst)]

Attachment: Updated possible fix.

Jesse, thanks for testing, and your merge was correct. This is likely to fix the new crash you're seeing. Please give this a try.

Comment 14

[Jesse Ruderman]

Addendum to the steps to reproduce in comment 10: after opening the first popup, make sure you don't have sound muted (link at the top of the popup).

Comment 15

[Jesse Ruderman]

The patch in comment 13 does the trick.  No crash, no trace-refcnt leaks.  Yay!

I do see one assertion failure soon after pressing Cmd+Q, but it could just the site (or Greasemonkey) tickling bug 324025 or bug 400349.

###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!: 'Error', file /Users/jruderman/trunk/mozilla/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 772

Comment 16

[Johnny Stenback (:jst)]

Attachment: Simpler version of the above.

This is the same fix as the previous attachment, just w/o some of the cleanup and code simplification, at the cost of duplicating those few lines of code. Probably safer to take this at this point rather than the above version.

Comment 17

[Johnny Stenback (:jst)]

Attachment: Fix that was checked in.

Same as above, with the last hunk of the more complicated patch included (needed to fix Jesse's second crash, somehow fell out of the simplified patch).

Comment 18

[Johnny Stenback (:jst)]

Fixed.

Comment 19

[Jesse Ruderman]

I have verified that the final patch fixes the crash for me on Mac.

Comment 20

[Tim Riley [:timr]]

I am moving the URL into a comment since it contains objectionable material with no ability to a disclaimer next to it. At least one person has expressed a concern about this site.  Don't go this this site if you want to avoid objectionable material.

http://www.myfreecams.com/?conn_mode=ajax

Comment 21

[Shane Blackett]

Changing linux and mac to the delayed plugin destruction scheme has
caused a number of plugins to crash.
Quicktime Bug 425157, Flip4Mac Bug 426524, XStandard Bug 430219 and
Zinc (our extension/plugin) Bug 429604.

As far as I understand it, the problem is not only with the plugins,
I don't see how NPAPI plugins can know when the window they have
is now invalid, either to deallocate resources associated with
the window before it is invalid (we use OpenGL and I have to set the
state with the correct handle while is is still valid to release resources)
or to ensure that we stop timed events (which I guess is where many of
these apps are falling over).
It is implied in this tracker that plugins that need to know when the 
window is no longer valid can observe the SetWindow(NULL); call,
this sounds suitable, however the ns4xPluginInstance code specifically
does not pass this call on, and to change is so that it did would 
presumably break many other plugins who aren't expecting it.
My understanding is that this is the preferred plugin code to be using.
  // XXX 4.x plugins don't want a SetWindow(NULL).
  if (!window || !mStarted)
    return NS_OK;
One possibility for fixing the problem would be to have a ns4xPluginInstance
setting that allows a plugin to request that these SetWindow(NULL) calls do 
in fact propagate.  I would be happy to try writing such as patch if others thought it might pass review?