393845 - Crash [@ nsObjectFrame::StopPluginInternal] with Windows Media Player 10 with this testcase

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, when clicking 1 or 2 times on the button while the movie is playing, Mozilla crashes. Probably because the Windows Media Player 10 is crashing.

This started crashing between 2005-09-21 and 2005-09-23, I guess somehow a regression of bug 1156.

It doesn't crash on my Windows Vista computer which has the Windows Media Player 11 installed.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

do you have a stacktrace/breakpad id?

Comment 2

[Martijn Wargers (dead)]

Ok, for some reason I do get a breakpad id now with the testcase online, I didn't get one offline.

http://crash-stats.mozilla.com/report/index/e995110f-54e6-11dc-b1c0-001a4bd43e5c
0  	nsObjectFrame::StopPluginInternal(int)  	 mozilla/layout/generic/nsObjectFrame.cpp:1575
1 	nsObjectFrame::PrepareInstanceOwner() 	mozilla/layout/generic/nsObjectFrame.cpp:1378
2 	nsObjectFrame::Instantiate(char const*, nsIURI*) 	mozilla/layout/generic/nsObjectFrame.cpp:1414
3 	nsObjectLoadingContent::Instantiate(nsIObjectFrame*, nsACString_internal const&, nsIURI*) 	mozilla/content/base/src/nsObjectLoadingContent.cpp:1502
4 	nsObjectLoadingContent::TryInstantiate(nsACString_internal const&, nsIURI*) 	mozilla/content/base/src/nsObjectLoadingContent.cpp:1471
5 	nsObjectLoadingContent::LoadObject(nsIURI*, int, nsCString const&, int) 	mozilla/content/base/src/nsObjectLoadingContent.cpp:991
6 	nsObjectLoadingContent::LoadObject(nsAString_internal const&, int, nsCString const&, int) 	mozilla/content/base/src/nsObjectLoadingContent.cpp:818
7 	nsHTMLSharedObjectElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, int) 	mozilla/content/html/content/src/nsHTMLSharedObjectElement.cpp:285
8 	nsGenericHTMLElement::SetAttrHelper(nsIAtom*, nsAString_internal const&) 	mozilla/content/html/content/src/nsGenericHTMLElement.cpp:2313
9 	nsHTMLSharedObjectElement::SetSrc(nsAString_internal const&) 	mozilla/content/html/content/src/nsHTMLSharedObjectElement.cpp:327
10 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
11 	AutoJSSuspendRequest::SuspendRequest() 	mozilla/js/src/xpconnect/src/xpcprivate.h:3317
12 	xul.dll@0x68218f

This testcase doesn't crash with the Windows Media Player 11, however I do have a testcase that also crashes with this kind of stacktrace with the Windows Media Player 11, though. (I have a bit of trouble getting it minimized further).

The stacktrace looks like it's related to bug 354102. I can't find it anymore on the topcrasher list, though.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

If you could attach even the non-minimized WMP11 testcase that would be great, since I don't have WMP10 available to test with here.

Comment 4

[Martijn Wargers (dead)]

Attachment: unminimized testcase that crashes with the windows media player 11 plugin

Ok, this is a not completely minimized testcase that crashes with the windows media player 11 for me with current trunk build, usually after 10 seconds or so. I hope it does the same for you.

Comment 5

[Martijn Wargers (dead)]

Oh, you need to download that testcase, using right-click Save As.. and also download the video file from http://martijn.martijn.googlepages.com/testmovie.wmv

Comment 6

[Martijn Wargers (dead)]

Feel free to open the testcase up, if you don't think it should remain security sensitive.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

OK, the original testcase crashes here (winxp home/wmp11).

0012d4a8 01f8d2b4 gklayout!nsPluginInstanceOwner::SetOwner+0xd [c:\mozilla\layout\generic\nsobjectframe.cpp @ 386]
0012d4c8 01f8ca13 gklayout!nsObjectFrame::StopPluginInternal+0xb4 [c:\mozilla\layout\generic\nsobjectframe.cpp @ 1639]
0012d4e0 01f8cc0a gklayout!nsObjectFrame::PrepareInstanceOwner+0x13 [c:\mozilla\layout\generic\nsobjectframe.cpp @ 1380]
0012d544 02529d62 gklayout!nsObjectFrame::Instantiate+0x4a [c:\mozilla\layout\generic\nsobjectframe.cpp @ 1414]
0012d578 02529bd8 gklayout!nsObjectLoadingContent::Instantiate+0x182 [c:\mozilla\content\base\src\nsobjectloadingcontent.cpp @ 1502]
0012d598 025280dd gklayout!nsObjectLoadingContent::TryInstantiate+0x98 [c:\mozilla\content\base\src\nsobjectloadingcontent.cpp @ 1472]
0012d7dc 02527828 gklayout!nsObjectLoadingContent::LoadObject+0x88d [c:\mozilla\content\base\src\nsobjectloadingcontent.cpp @ 991]
0012d870 0251aab0 gklayout!nsObjectLoadingContent::LoadObject+0x1c8 [c:\mozilla\content\base\src\nsobjectloadingcontent.cpp @ 818]
0012d8ec 02171129 gklayout!nsHTMLSharedObjectElement::SetAttr+0x70 [c:\mozilla\content\html\content\src\nshtmlsharedobjectelement.cpp @ 286]
0012d90c 0217c5fb gklayout!nsGenericHTMLElement::SetAttr+0x29 [c:\mozilla\content\html\content\src\nsgenerichtmlelement.h @ 213]
0012d928 0251af19 gklayout!nsGenericHTMLElement::SetAttrHelper+0x1b [c:\mozilla\content\html\content\src\nsgenerichtmlelement.cpp @ 2314]
0012d938 00305137 gklayout!nsHTMLSharedObjectElement::SetSrc+0x19 [c:\mozilla\content\html\content\src\nshtmlsharedobjectelement.cpp @ 327]
0012d94c 00feb1e3 xpcom_core!NS_InvokeByIndex_P+0x27 [c:\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp @ 102]



   384:   void SetOwner(nsObjectFrame *aOwner)
   385:   {
>  386:     mOwner = aOwner;

|this| is null... (and so's aOwner)

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

OK, this is a fun (?) bug.

So StopPluginInternal gets reentered:
gklayout!nsObjectFrame::StopPluginInternal+0x32
gklayout!nsObjectFrame::Destroy+0x4f
gklayout!nsBlockFrame::DoRemoveFrame+0x41f
gklayout!nsBlockFrame::RemoveFrame+0x32
gklayout!nsFrameManager::RemoveFrame+0x3f
gklayout!nsCSSFrameConstructor::ContentRemoved+0x413
gklayout!nsCSSFrameConstructor::RecreateFramesForContent+0xf5
gklayout!nsCSSFrameConstructor::RestyleElement+0x9b
gklayout!nsCSSFrameConstructor::ProcessOneRestyle+0x9c
gklayout!nsCSSFrameConstructor::ProcessPendingRestyles+0x163
gklayout!PresShell::DoFlushPendingNotifications+0xcf
gklayout!PresShell::WillPaint+0x39
gklayout!nsViewManager::DispatchEvent+0x38b
gklayout!HandleEvent+0x46
gkwidget!nsWindow::DispatchEvent+0xc1
gkwidget!nsWindow::DispatchWindowEvent+0x24
gkwidget!nsWindow::OnPaint+0x694
gkwidget!nsWindow::ProcessMessage+0x6aa
gkwidget!nsWindow::WindowProc+0x142
USER32!InternalCallWinProc+0x28
USER32!UserCallWinProcCheckWow+0x150
USER32!DispatchClientMessage+0xa3
USER32!__fnDWORD+0x24
ntdll!KiUserCallbackDispatcher+0x13
USER32!NtUserDispatchMessage+0xc
USER32!DispatchMessageW+0xf
WARNING: Stack unwind information not available. Following frames may be wrong.
wmp!Ordinal3003+0x8b902
wmp!Ordinal3002+0x116aae
wmp!Ordinal3003+0x860eb
wmp!Ordinal3002+0x88bf3
wmp!Ordinal3003+0xacf0b
wmp!Ordinal3003+0xadafb
wmp!DllGetClassObject+0xa2a0
wmpdxm!CWMPShim::SetClientSite+0xb7
npdsplay!unuse_netscape_plugin_Plugin+0x1a4e
npdsplay!native_NPDS_npDSJavaPeer_StreamSelect+0x25b8
npdsplay!NP_Shutdown+0x1fd
gkplugin!ns4xPluginInstance::Stop+0x1bb
gklayout!DoStopPlugin+0x195
gklayout!nsObjectFrame::StopPluginInternal+0xdb
gklayout!nsObjectFrame::PrepareInstanceOwner+0x25
gklayout!nsObjectFrame::Instantiate+0x4a
gklayout!nsObjectLoadingContent::Instantiate+0x182
gklayout!nsObjectLoadingContent::TryInstantiate+0x98
gklayout!nsObjectLoadingContent::LoadObject+0x88d
gklayout!nsObjectLoadingContent::LoadObject+0x1c8
gklayout!nsHTMLSharedObjectElement::SetAttr+0x70
gklayout!nsGenericHTMLElement::SetAttr+0x29
gklayout!nsGenericHTMLElement::SetAttrHelper+0x1b
gklayout!nsHTMLSharedObjectElement::SetSrc+0x19

So we stop the plugin, which processes events (!?), we get a paint event, attempting to paint flushes the pending notifications (there's a pending restyle event due to the setting of position to absolute), which destroys the frame, which calls StopPluginInternal again.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

btw, the reason why this works pre-1156 is that setting src didn't have any effect, and once the new frame got constructed it just instantiated the plugin again.

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: slightly different testcase

A similar testcase (changes display to none instead of changing position)

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, the usual "Windows processes events any time you destroy a widget" thing?  Fun times.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 278696 [details] [diff] [review]
patch

>Index: layout/generic/nsObjectFrame.cpp
>Index: content/base/src/nsObjectLoadingContent.cpp
>+      // We came here via eState_Loading. Therefore, our frame if newly

s/if/is/

>+  // Stop the plugin first. As that might destroy the frame,

s/As/Since/

r+sr=bzbarsky

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in content/base/src/nsObjectLoadingContent.cpp;
/cvsroot/mozilla/content/base/src/nsObjectLoadingContent.cpp,v  <--  nsObjectLoadingContent.cpp
new revision: 1.62; previous revision: 1.61
done
Checking in layout/generic/nsObjectFrame.cpp;
/cvsroot/mozilla/layout/generic/nsObjectFrame.cpp,v  <--  nsObjectFrame.cpp
new revision: 1.615; previous revision: 1.614
done

Comment 15

[Martijn Wargers (dead)]

Verified fixed using WMP11 and windows vista:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007090604 Minefield/3.0a8pre

Also verified fixed with that same dated build on windowsXP using WMP10.
No crash anymore with any of the testcases.

Nice job!

Comment 16

[Gerd]

Sorry Folks,

but it seems that this bug is not fully fixed.

We have another crash related to Java with AdBlock Plus 0.7.5.1 installed.

Steps to reproduce:
With AdBlock Plus 0.7.5.1 installed, go to http://www.java.com/en/download/installed.jsp and click on the green "Verify Installation" button.

Regressionwindow:
Works as expected in 20070905_1328
Broken in 20070905_1344 (crash)

Breakpad:
http://crash-stats.mozilla.com/report/index/da4932d7-5d5e-11dc-96ec-001a4bd46e84
http://crash-stats.mozilla.com/report/index/3d3753e3-5d5e-11dc-a943-001a4bd43ef6

Comment 17

[Martijn Wargers (dead)]

Gerd, could you file a new bug on that and make it blocking this bug? Thanks.

Comment 18

[Gerd]

OK, filed 
Bug 395412 – Crash on Java install verification with AdBlock Plus 0.7.5.1 installed

Comment 19

[Steve England [:stevee]]

It looks like this patch caused bug 397051

Comment 20

[Martijn Wargers (dead)]

Maybe this patch should be backed out for now, because of the regressions it's causing?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

I've backed this patch out per IRC discussion with Peter and Christian.

mozilla/layout/generic/nsObjectFrame.cpp 	1.616
mozilla/content/base/src/nsObjectLoadingContent.cpp 	1.64 

Comment 22

[Martijn Wargers (dead)]

Any chance for a patch without causing the regressions?

Comment 23

[Martijn Wargers (dead)]

Attachment: stacktraces from testing with latest Java plugin

While trying to come up with useful stacktraces for bug 275783 (see bug, comment 113 and further), I got these stacktraces, which seems similar to this bug.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Martijn, as far as I can see the frames are different from the ones given when filing this bug. I think this should be better handled in bug 275783?

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The crash with the mentioned testcase in comment still happens on latest nightly builds under Windows XP. Here an updated crasher id: bp-78cfaf3a-e000-11dc-a569-001a4bd43e5c

Comment 26

[Martijn Wargers (dead)]

(In reply to comment #24)
> Martijn, as far as I can see the frames are different from the ones given when
> filing this bug. I think this should be better handled in bug 275783?

I'm not sure, I'm probably just making a mess of thing here and in the other bug :(

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Biesi, do you have time to work on this bug? Otherwise we should search for a new assignee. Thanks.

Comment 28

[Johnny Stenback (:jst)]

Things might very well have improved here with bug 410946 fixed. Could someone test with a nightly from tomorrow or later?

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Martijn, the attached testcase doesn't work anymore. The referenced video has been deleted. Do we have another one?

Comment 30

[Martijn Wargers (dead)]

Henrik, for me the video is still there:
http://martijn.martijn.googlepages.com/testmovie.wmv
And I still crash with the attached testcase (at least with WMP 10):
http://crash-stats.mozilla.com/report/index/de9055e6-e6ec-11dc-aba0-001a4bd43ef6

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Martijn, your build id is still an old one. But I also crashed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008022906 Minefield/3.0b4pre.

The stack looks totally different now. It looks like that I had a recursion:
bp-871c50f8-e6f3-11dc-bb47-001a4bd43ed6.

Comment 32

[Johnny Stenback (:jst)]

Attachment: Possible fix.

Martijn, this should fix the crash you referenced in your last comment. Any chance you could give this a try (let me know if you need help getting a build with this patch)? Not sure what's up with the stack overflow that Henrik was seeing, but let's try this patch first and see if it helps.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Johnny, I started a TryServer build so we could run a test in about 2 hours. I'm not able to build my own debug build under Windows right now. I'll post the download link when the build is ready.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry, this wont work. Johnny handed out a gid patch. Any change to get a cvs diff?

Comment 35

[Johnny Stenback (:jst)]

Henrik, just set "patch level" (I think that's what it's called) to 1 instead of 0 when you upload the patch to the try server, that should make it apply. Thanks for spinning builds!

Comment 36

[Martijn Wargers (dead)]

Well, I'm getting build errors with the patch, so I can't test it:
http://martijn.martijn.googlepages.com/builderrors.txt

Comment 37

[Johnny Stenback (:jst)]

Attachment: Possible fix that builds, even.

Duh. How about this?

Comment 38

[Martijn Wargers (dead)]

Yeah, thanks, that one compiles fine and seems to fix the crash (at least on windowsXP with Media Player 10).

Comment 39

[Johnny Stenback (:jst)]

Comment on attachment 307741 [details] [diff] [review]
Possible fix that builds, even.

Awesome, thanks for testing. bz, what do you think of this?

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

TryServer builds are also ready:
https://build.mozilla.org/tryserver-builds/2008-03-06_10:[e-mail]-bug393845-crasher/

I'll run on my testbox to see if my issue gets also fixed by your patch.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Johnny, it also fixes my issue. No crash anymore.

Comment 42

[Johnny Stenback (:jst)]

Awesome, thanks for testing, both of you!

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 307741 [details] [diff] [review]
Possible fix that builds, even.

Makes sense.  Maybe add some comments here that the order is very important?

Comment 44

[Johnny Stenback (:jst)]

Attachment: Fix that was checked in, with comments added.

Comment 45

[Johnny Stenback (:jst)]

Fixed.

Comment 46

[jag (Peter Annema)]

>diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp

>+  // Grab the view while we still know it's safe to do so.
>+
>+  nsIView *view = GetView();
> 
> #ifdef XP_WIN
>   if (aDelayedStop) {
>     // If we're asked to do a delayed stop it means we're stopping the
>     // plugin because we're destroying the frame. In that case, tell
>     // the view to disown the widget (i.e. leave it up to us to
>     // destroy it).
>+
>     nsIView *view = GetView();
>     if (view) {
>       view->DisownWidget();
>     }
>+  }
>+#endif

I'm probably missing something, but why are you grabbing the view outside the XP_WIN ifdef? Or, why are you grabbing it again inside it?

Comment 47

[Johnny Stenback (:jst)]

Um, yeah, that was dumb. I just checked in a fix, no need to get the view outside that if check at all now. Thanks jag, good catch!

Comment 48

[Martijn Wargers (dead)]

Verified fixed.
The first testcase doesn't crash anymore with current trunk build on windows XP with Windows Media Player 10.
The second testcase doesn't crash anymore with current trunk build on windos Vista with Windows Media Player 11 (while it crashed with yesterday's trunk build).

Thank you for fixing this!

Comment 49

[Martijn Wargers (dead)]

In comment 23, I mentioned a crash in combination with the latest Java plugin, which I thought that might have something to do with this bug.
It turns out the fix for this crash hasn't fixed that, so I filed bug 421833 for it.