347743 - (trunk) Crash when closing tab with windows media player with onblur handler that removes plugin object

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Martijn Wargers (dead)]

This is the trunk version of the crash as mentioned in bug 347742.
The testcase is also from that bug.

Talkback ID for trunk: TB21861563G
nsPropertyTable::PropertyList::Equals   nsPropertyTable::GetProperty   nsPluginInstanceOwner::Destroy   nsObjectFrame::StopPlugin   nsObjectFrame::Destroy   nsBlockFrame::RemoveFrame   nsFrameManager::RemoveFrame   0x0012e7bc

Comment 1

[Martijn Wargers (dead)]

Some other way of crashing with the windows media player, might be useful for testing, since you're busy with bug 347662 anyway. See also bug 347742.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Erm.... nsPluginInstanceOwner::Destroy doesn't call nsPropertyTable::GetProperty.

Does this still appear with the band-aid from bug 347662?  Can you reproduce this in a debug build?

Comment 3

[Martijn Wargers (dead)]

Yes, this still happens with the band-aid from bug 347662 (although I had to more or less 'invent' how to apply the patch on trunk).
I think I am able to reproduce this in gdb.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Jonas, is there a reason this crash isn't even wanted?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Put another way, why's a security bug not blocking and not wanted?  ;)

Comment 6

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

oops, didn't realize this was that bad. We'll look at it next triage again.

Comment 7

[chris hofmann]

does this still look like its making 1.9?

Comment 8

[Johnny Stenback (:jst)]

Attachment: Fix, move plugin destruction out of frame destruction code.

This fixes the bug on Win32 (only, and AFAIK it's only a problem with the Windows Media Player plugin) by making the plugin get destroyed outside of layout frame destruction where it's not safe to call into plugin code. diff -w coming up for review.

Comment 9

[Johnny Stenback (:jst)]

Attachment: diff -w of the above.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 268873 [details] [diff] [review]
diff -w of the above.

This aint ready yet.

Comment 11

[Johnny Stenback (:jst)]

Attachment: Fix, move plugin destruction out of frame destruction code.

This is the same as the first attachment here with some fixes, mainly to reparent the plugin widget to the top-level firefox window (per roc's suggestion). To do that I needed to use the native Win32 API as there's no widget level API that lets me find the right native window. diff -w coming up for review.

Comment 12

[Johnny Stenback (:jst)]

Attachment: diff -w of the above.

Comment 13

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

   virtual void StopPlugin();
+  void StopPlugin(PRBool aDelayedStop);

Slightly confusing. Call it StopPluginInternal, say, and document what aDelayedStop does.

doStopPlugin should be DoStopPlugin.

+    // Pass ownership of mInstanceOwner to nsStopPluginRunnable

Since it's refcounted, talking about "passing ownership" is slightly misleading. For a little while, both own it.

+  PRPackedBool                mDestroyWidget;

Can you document this?

+/*
+ * Copy of GetNSWindowPropName() from widget/src/windows/nsWindow.cpp
+ */

This is a bit nasty. Is there a way to avoid copying the code, perhaps by adding a new method to nsIWidget to get the nsIWidget from a void* native handle? A kind of reverse of GetNativeData. It could be stubbed out on non-Windows. Alternatively, an API to find the toplevel nsIWidget by scanning the native hierarchy.

nsIWidget lifetimes worry me. You're relying on the plugin's nsIWidget not holding/using a pointer to its parent. That seems to be OK but please document it.

+    // the top-level firefox widget so that we can safely tear down

"Gecko" not "firefox" :-)

I wonder if moving the plugin widget up could do weird things ... e.g. should we disable its window to avoid it getting focus?

Comment 14

[Johnny Stenback (:jst)]

Attachment: Updated fix.

Comment 15

[Johnny Stenback (:jst)]

Attachment: Updated fix. (diff -w)

Roc, this should address all those comments. Let me know if you come across something that doesn't, or you think of something else...

Comment 16

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

Comment on attachment 269760 [details] [diff] [review]
Updated fix. (diff -w)

+    /*
+     * Return the widget at the top of the widget hierarcy for the
+     * running application.
+     */
+    virtual nsIWidget* GetAppTopWidget(void) = 0;

I'm not super enthusiastic about this name. Perhaps call it GetTopLevelWindow() and explain that it's the nearest ancestor the widget which does not have a Gecko parent?

Comment 17

[Johnny Stenback (:jst)]

Attachment: Patch that was checked in.

Done. This patch changes the name. To do that I had to rename the internal nsWindow::GetTopLevelWindow() method (which unfortunately doesn't do what I want) to GetTopLevelWindowInternal().

Comment 18

[Johnny Stenback (:jst)]

Fix checked in.

Comment 19

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070628 Minefield/3.0a6pre

Thanks for fixing this one!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Attachment: Backout patch

This backs out jst's commit and fixes the crashing reported in bug 386332

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Attachment: Backout patch round 2

Whoops, don't know how the DOS line endings got in there...

Comment 22

[Mike Connor [:mconnor]]

Backed out so we don't ship in a6 with the crashing

Comment 23

[Eli Friedman]

The cause of the crash is that nsWindow::SetParent is broken: it reparents the native widget, but not itself (the nsIWidget).  This has extremely bad consequences because it means the nsIWidget is not guaranteed to be destroyed at the same time as its parent, which breaks the nsIWidget destruction code and ultimately causes us to access freed memory, leading to the crash in bug 386332.

Comment 24

[Boris Zbarsky [:bzbarsky]]

It looks like the GTK2 SetParent is broken as well?

Comment 25

[Eli Friedman]

I think we might have to give up on this approach, though; even with SetParent fixed, I managed to trigger an internal crash in Flash once. (It's also possible that fixing SetParent isn't enough; I'm getting an extremely odd shutdown crash I haven't been able to figure out.)

Comment 26

[Eli Friedman]

(In reply to comment #24)
> It looks like the GTK2 SetParent is broken as well?

Probably; it doesn't matter at the moment because we don't call it, though.

Comment 27

[Eli Friedman]

Attachment: Fix for SetParent (for reference)

Comment 28

[Johnny Stenback (:jst)]

(In reply to comment #25)
> I think we might have to give up on this approach, though; even with SetParent
> fixed, I managed to trigger an internal crash in Flash once. (It's also
> possible that fixing SetParent isn't enough; I'm getting an extremely odd
> shutdown crash I haven't been able to figure out.)
> 

Are you getting that shutdown crash consistently? If so, any thoughts on how to reproduce this?

Comment 30

[Eli Friedman]

Comment on attachment 270349 [details] [diff] [review]
Fix for SetParent (for reference)

I've been asked to ask for review on this patch... I believe it's correct, and it's guaranteed not to break anything because we never call SetParent().   (If we did, we would have run into this problem a while ago.)

Another possibility worth considering would be switching to delayed destruction of views.  Essentially, the idea would be that we stick all the views we want to destroy into an array or something, and then destroy the views once we're out of frame construction.  That solution wouldn't require any fancy widget tricks, and it should fix this problem by not dropping into widget code at dangerous times.

Comment 31

[Eli Friedman]

Ugh, I really don't know what to do here...

The patch that was checked in causes crashes on shutdown, for some reason I haven't figured out.

I don't think there's any other feasible possibility, though: we can't delay widget destruction because of parent widgets, but we can't destroy the plugin's widget outside of an event specifically for that purpose because we can process arbitrary events from the event queue (like mouse events) during the destruction of the plugin's widget!

Comment 32

[Eli Friedman]

Attachment: Fixed patch (WIP)

This version seems to work without crashing.

It is a little bit weird to be reparenting out plugin widgets to the desktop, but it works just fine as far as I can tell.

Comment 33

[Eli Friedman]

Attachment: Fixed Patch

Version without compile error.  I'd appreciate some testing on this, since plugin stuff tends to be sensitive.

Comment 34

[Eli Friedman]

(Ignore the change to ForceRedraw.)

Comment 35

[Martijn Wargers (dead)]

I tried the patch, but I crash even befor the profile manager comes up:
http://martijn.martijn.googlepages.com/bt.txt
I did a make -f client.mk build_all_depend, or should I have done a make -f client.mk distclean before that?

Comment 36

[Eli Friedman]

I have no clue how you're crashing there.

Comment 37

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

Comment on attachment 272834 [details] [diff] [review]
Fixed Patch

This looks alright, but we'd better find out what Martijn's issue is.

Comment 38

[Martijn Wargers (dead)]

Ok, just applying the view part of the patch is enough to make my debug build crash, for some reason.

Comment 39

[Martijn Wargers (dead)]

Sorry for the disturbance.
It's working fine now, after I have rebuilt my tree from scratch (I also had some stuff cleaned up, that might have caused it).

Comment 40

[Martijn Wargers (dead)]

Ok, this is something that seems to crash because of this patch:
http://martijn.martijn.googlepages.com/flash_crash.htm
It crashes for me within 10 seconds normally after the iframe has disappeared.

I see this assertion in my debug build:
###!!! ASSERTION: dangling entry->mJSObj JSPrivate because we can't find cx: 'Er
ror', file c:/mozilla/mozilla/modules/plugin/base/src/nsJSNPRuntime.cpp, line 15
94

The iframe content is basically this:
<script>
function doe(i){
document.embeds[0].removeAttribute('style');
}
setTimeout(doe,200,0);
</script>
<embed type="application/x-shockwave-flash" style=" float: right;">

Comment 41

[Eli Friedman]

jst, would you mind looking at the above?  I'm not really familiar with the plugin code.

(Note that it crashes in cycle collection.)

Comment 42

[Johnny Stenback (:jst)]

Attachment: Fix for Martijn's crash.

The reasons for the crash Martijn was seeing was that now the plugin destruction can be delayed long enough for the JS context not being reachable from the plugins npp by the time the plugin is destroyed. That led to us not being able to null out the plugin's NPObject wrappers private data, which then during GC caused a crash due to us touching the deleted NPObject. This patch fixes that.

Comment 43

[Eli Friedman]

Comment on attachment 274244 [details] [diff] [review]
Fix for Martijn's crash.

Redirecting review request to someone who knows the plugin code.

Comment 44

[Brendan Eich [:brendan]]

Comment on attachment 274244 [details] [diff] [review]
Fix for Martijn's crash.

Redirecting to jst.

/be

Comment 45

[Martijn Wargers (dead)]

(In reply to comment #44)
> (From update of attachment 274244 [details] [diff] [review])
> Redirecting to jst.

Since Johnny wrote the patch himself, I'm quite sure he'll approve it ;)

Comment 46

[Brendan Eich [:brendan]]

(In reply to comment #45)
> (In reply to comment #44)
> > (From update of attachment 274244 [details] [diff] [review] [details])
> > Redirecting to jst.
> 
> Since Johnny wrote the patch himself, I'm quite sure he'll approve it ;)

Suits me! ;-)

Ok, I missed that. Looking now.

/be

Comment 47

[Brendan Eich [:brendan]]

Comment on attachment 274244 [details] [diff] [review]
Fix for Martijn's crash.

>+  NppAndCx *nppcx = static_cast<NppAndCx *>(arg);

I'm not a C++ purist -- I don't even play one on TV -- but isn't reinterpret_cast more the "right thing" here?

>+  JSContext *cx = GetJSContext(npp);
>+  if (!cx) {
>+    nsCOMPtr<nsIThreadJSContextStack> stack =
>+      do_QueryInterface(sContextStack);
>+
>+    if (!stack) {
>+      stack = do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>+      if (!stack) {
>+        // Nothing we can do if we get here.
>+        return;
>+      }
>+    }
>+
>+    stack->GetSafeJSContext(&cx);
>+    if (!cx) {
>+      // Nothing we can do if we get here.
>+    }
>+  }
>+
>   if (sNPObjWrappers.ops) {
>+    NppAndCx nppcx = { npp, cx };
>     PL_DHashTableEnumerate(&sNPObjWrappers,
>-                           NPObjWrapperPluginDestroyedCallback, npp);
>+                           NPObjWrapperPluginDestroyedCallback, &nppcx);

So is there a possibility that we'll have objects alive in the runtime with dangling private data pointers, but no ability to find a cx to use here? If so, and if the case does not involve running from the last GC when control flow reaches here, then I don't see how that bad case could arise. But it seems we could just always use the safe context, and shrink this code.

(Actually, we could also violate the JS API and use LOCKED_OBJ_SET_SLOT, which requires no cx at all.)

Thoughts? r=me in any event, but if you can shrink code please do.

/be

Comment 48

[Johnny Stenback (:jst)]

(In reply to comment #47)
> (From update of attachment 274244 [details] [diff] [review])
> >+  NppAndCx *nppcx = static_cast<NppAndCx *>(arg);
> 
> I'm not a C++ purist -- I don't even play one on TV -- but isn't
> reinterpret_cast more the "right thing" here?

Sure.

> So is there a possibility that we'll have objects alive in the runtime with
> dangling private data pointers, but no ability to find a cx to use here? If so,

Well, yes, we can run into a situation where we're unable to find the cx associated with the plugin due to its document already having been torn down (especially with the other patch in this bug applied).

> and if the case does not involve running from the last GC when control flow
> reaches here, then I don't see how that bad case could arise. But it seems we
> could just always use the safe context, and shrink this code.

This code rarely (if ever) runs from GC. But yeah, I could always use the safe context here (but the code doesn't really shrink much if I do).

Updated patch to that effect coming up.

Comment 49

[Johnny Stenback (:jst)]

Attachment: Updated fix for Martijn's crash.

Comment 50

[Martijn Wargers (dead)]

The patches in this bug can land, not? Or should I do some more testing before landing?

Comment 51

[Eli Friedman]

I landed "Updated fix for Martijn's crash"; it apparently caused Bug 391182.

Comment 52

[Martijn Wargers (dead)]

Ok, "Updated fix for Martijn's crash" landed at 2007-08-06 10:19.
Shouldn't https://bugzilla.mozilla.org/attachment.cgi?id=272834 ("Fixed patch") also land at some point?

Comment 53

[Eli Friedman]

Well, I don't know how important the assertion in bug 391182 is; if it's important enough to get this fix backed out, we obviously don't want to land the other part.

Comment 54

[Johnny Stenback (:jst)]

Eli, I'm fixing bug 391182, it's a bogus assertion after the fix you landed that I didn't catch in my testing. Please go ahead and land the fix for this bug.

Comment 55

[Eli Friedman]

Checked in.

Comment 56

[Martijn Wargers (dead)]

Thanks to all people who helped fixing this!

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082111 Minefield/3.0a8pre