Closed Bug 393845 Opened 14 years ago Closed 14 years ago

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

Categories

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

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta5

People

(Reporter: martijn.martijn, Assigned: jst)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(6 files, 1 obsolete file)

Attached file 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.
do you have a stacktrace/breakpad id?
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.
Summary: Crash with Window Media Player 10 with this testcase → Crash [@ nsObjectFrame::StopPluginInternal] with Window Media Player 10 with this testcase
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.
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.
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
Feel free to open the testcase up, if you don't think it should remain security sensitive.
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)
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.
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.
A similar testcase (changes display to none instead of changing position)
Attached patch patchSplinter Review
Assignee: nobody → cbiesinger
Status: NEW → ASSIGNED
Attachment #278696 - Flags: superreview?(bzbarsky)
Attachment #278696 - Flags: review?(bzbarsky)
Oh, the usual "Windows processes events any time you destroy a widget" thing?  Fun times.
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
Attachment #278696 - Flags: superreview?(bzbarsky)
Attachment #278696 - Flags: superreview+
Attachment #278696 - Flags: review?(bzbarsky)
Attachment #278696 - Flags: review+
Attachment #278696 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M8
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!
Status: RESOLVED → VERIFIED
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
Gerd, could you file a new bug on that and make it blocking this bug? Thanks.
OK, filed 
Bug 395412 – Crash on Java install verification with AdBlock Plus 0.7.5.1 installed
Depends on: 395412
Blocks: 396284
No longer blocks: 396284
Depends on: 396284
Depends on: 397051
It looks like this patch caused bug 397051
Depends on: 396714
Maybe this patch should be backed out for now, because of the regressions it's causing?
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 
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Blocks: 404616
Any chance for a patch without causing the regressions?
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
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.
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?
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

Target Milestone: mozilla1.9alpha8 → ---
(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 :(
Biesi, do you have time to work on this bug? Otherwise we should search for a new assignee. Thanks.
Assignee: cbiesinger → jst
Status: REOPENED → NEW
Flags: tracking1.9+ → blocking1.9+
Priority: P3 → P2
Things might very well have improved here with bug 410946 fixed. Could someone test with a nightly from tomorrow or later?
Martijn, the attached testcase doesn't work anymore. The referenced video has been deleted. Do we have another one?
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
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.
Attached patch Possible fix. (obsolete) — Splinter Review
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.
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.
Status: NEW → ASSIGNED
Sorry, this wont work. Johnny handed out a gid patch. Any change to get a cvs diff?
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!
Well, I'm getting build errors with the patch, so I can't test it:
http://martijn.martijn.googlepages.com/builderrors.txt
Duh. How about this?
Attachment #307730 - Attachment is obsolete: true
Yeah, thanks, that one compiles fine and seems to fix the crash (at least on windowsXP with Media Player 10).
Comment on attachment 307741 [details] [diff] [review]
Possible fix that builds, even.

Awesome, thanks for testing. bz, what do you think of this?
Attachment #307741 - Flags: superreview?(bzbarsky)
Attachment #307741 - Flags: review?(bzbarsky)
TryServer builds are also ready:
https://build.mozilla.org/tryserver-builds/2008-03-06_10:09-mozilla@hskupin.info-bug393845-crasher/

I'll run on my testbox to see if my issue gets also fixed by your patch.
Johnny, it also fixes my issue. No crash anymore.
Awesome, thanks for testing, both of you!
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?
Attachment #307741 - Flags: superreview?(bzbarsky)
Attachment #307741 - Flags: superreview+
Attachment #307741 - Flags: review?(bzbarsky)
Attachment #307741 - Flags: review+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
>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?
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!
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!
Status: RESOLVED → VERIFIED
Summary: Crash [@ nsObjectFrame::StopPluginInternal] with Window Media Player 10 with this testcase → Crash [@ nsObjectFrame::StopPluginInternal] with Windows Media Player 10 with this testcase
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.
Depends on: test-plugin
Flags: in-testsuite?
Crash Signature: [@ nsObjectFrame::StopPluginInternal]
You need to log in before you can comment on or make changes to this bug.