Closed Bug 416953 Opened 14 years ago Closed 14 years ago

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

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: jst)

References

Details

(Keywords: assertion, crash, Whiteboard: [sg:critical])

Attachments

(6 files)

Attached file 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)
...
Flags: blocking1.9?
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...
Bob, please file a new bug on your crash, along with specific steps to reproduce. Thanks.
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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.
Whiteboard: [sg:critical]
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".
Flags: tracking1.9+ → blocking1.9+
Assignee: nobody → 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...
Attached patch Possible fix.Splinter Review
Jesse, or anyone else, could you give this a try? This is totally untested, but does compile here.
Builds with the attached fix (merged to trunk) available here:

https://build.mozilla.org/tryserver-builds/2008-03-08_23:16-jst@mozilla.com-flash-crash/

Jesse, or anyone with a mac, please give these builds a spin and try to reproduce this if possible.
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.
Btw, I merged the last hunk of your patch to trunk as

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

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.
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).
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
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.
Attachment #309289 - Flags: superreview?(jonas)
Attachment #309289 - Flags: review?(jonas)
Attachment #309289 - Flags: superreview?(jonas)
Attachment #309289 - Flags: superreview+
Attachment #309289 - Flags: review?(jonas)
Attachment #309289 - Flags: review+
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).
Fixed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
I have verified that the final patch fixes the crash for me on Mac.
Depends on: 423260
Depends on: 423446
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
Depends on: 425157
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?
Depends on: 426524, 429604, 430219
Group: core-security
You need to log in before you can comment on or make changes to this bug.