507926 - Crash [@ nsPluginInstanceOwner::CreateWidget] with quicktime plugin and changing styles

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, I usually crash with that testcase after 20 seconds or so. It usually seems to happen after minimizing the window or something like that.

The content of the iframe is this:
<?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" xmlns:html="http://www.w3.org/1999/xhtml">
<iframe xmlns="http://www.w3.org/1999/xhtml" style="display: table;" onDOMAttrModified="this.focus()" src="data:video/quicktime;charset=utf-8,"/>

<script xmlns="http://www.w3.org/1999/xhtml">
function doe() {
document.getElementsByTagName('*')[1].setAttribute('style', '');
document.documentElement.setAttribute('style', 'overflow: scroll;');
}
setTimeout(doe,100);
</script>
</window>

This seems to have regressed between 2009-07-20 and 2009-07-26.

http://crash-stats.mozilla.com/report/index/6bd06a93-2816-41b0-bab0-9c18b2090802?p=1
0  	xul.dll  	xul.dll@0x3d0830  	
1 	xul.dll 	nsPluginInstanceOwner::CreateWidget 	layout/generic/nsObjectFrame.cpp:4977
2 	xul.dll 	nsPluginHost::InstantiateEmbeddedPlugin 	modules/plugin/base/src/nsPluginHost.cpp:3235
3 	xul.dll 	nsObjectFrame::InstantiatePlugin 	layout/generic/nsObjectFrame.cpp:907
4 	xul.dll 	nsObjectFrame::Instantiate 	layout/generic/nsObjectFrame.cpp:2008
5 	xul.dll 	nsObjectLoadingContent::Instantiate 	content/base/src/nsObjectLoadingContent.cpp:1762
6 	xul.dll 	nsAsyncInstantiateEvent::Run 	content/base/src/nsObjectLoadingContent.cpp:155
7 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:527
8 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
9 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:193
10 	nspr4.dll 	PR_GetEnv 	
11 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:110
12 	firefox.exe 	firefox.exe@0x21a7 	
13 	kernel32.dll 	kernel32.dll@0x17076 	

The offending code seems to indicate this is a regression from bug 339548.

Comment 1

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

Can you reproduce on Mac? Or with Flash or the test plugin?

Comment 2

[Martijn Wargers (dead)]

I couldn't reproduce it with Flash. How do I invoke the test plugin?
I haven't tried Mac yet.

Comment 3

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

I guess you have to make a build with --enable-tests. Then you want something like this:

<embed type="application/x-test" wmode="window"></embed>

But if it's much work then I should just install Quicktime on Windows and try your testcase.

Comment 4

[Martijn Wargers (dead)]

I haven't been able to reproduce the crash in a debug build at all, so testing with the test plugin didn't work either. (using an <embed> doesn't crash either with the quicktime plugin, either).
I've now tried the testcase on the Mac, it doesn't seem to crash there.

Sorry, it seems to be limited to the Quicktime plugin on windows. I guess that plugin is doing something weird. 

Btw, I'm seeing this assertion in a debug build with the testcase:
###!!! ASSERTION: Wrong scope, this is really bad!: 'JS_GetGlobalForObject(cx, o
bj) == newScope', file c:/mozilla-build-1.3/src/content/base/src/nsDocument.cpp,
 line 3569
Maybe related?

Is there a way to install the test plugin in regular builds, btw? (I guess not)

Comment 5

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

You could probably just copy it over.

Comment 6

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

I'm not sure how we could crash here:
http://hg.mozilla.org/mozilla-central/annotate/8366e5cc9f57/layout/generic/nsObjectFrame.cpp#l4977
mPluginWindow can't be null because we've checked at the top of the function and nothing much has happened on the path to line 4977. If mOwner was null we'd have crashed earlier. mOwner->PresContext() can't ever return null. So we must have really crashed inside CreateWidget ... but where?

Comment 7

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

But wait! Letting the testcase run in my debug build actually did trigger the crash!

>	gklayout.dll!nsRootPresContext::UpdatePluginGeometry(nsIFrame * aChangedSubtree=0x06a9e708)  Line 2333 + 0x7 bytes	C++
 	gklayout.dll!nsObjectFrame::CreateWidget(int aWidth=600, int aHeight=600, int aViewOnly=0)  Line 721	C++
 	gklayout.dll!nsPluginInstanceOwner::CreateWidget()  Line 5039 + 0x35 bytes	C++

We're at
  widget->ConfigureChildren(configurations);
and 'widget' is null.

Further testing suggests CreateWindowExW in nsWindow::StandardWindowCreate is failing.

Comment 8

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

And that's failing because "parent" is null. And that's because DocumentViewerImpl::MakeWindow has null mParentWidget and null mContainerView, even though we seem to be constructing the presentation in the testcase's IFRAME, which is an eContentTypeContentFrame, so the subdocument should be windowless and mContainerView should be set!

Comment 9

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

Er, that's aContainerView

Comment 10

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

Attachment: fix

This *seems* to fix it. I've been running the test for several minutes now with no crash, whereas I could reproduce it quite easily before.

We have to be able to handle cases where we have no parent widget and no view to hook up to --- external resource documents depend on it, and so do some edge cases where a <frame> element is not actually being displayed as a frame.

In those cases we're supposed to create an invisible root widget for the document. But on Windows, the actual CreateWindowEx call was failing, because we were passing WS_CHILDWINDOW style in but no parent HWND. This left mWnd null, which caused a chain of cascading failures --- we'd fail to create an mWnd for the plugin's widget, which would then cause GetParent() to return null since we can't walk the HWND chain to find the parent nsIWidget, which caused the crash.

Comment 11

[Masatoshi Kimura [:emk]]

> In those cases we're supposed to create an invisible root widget for the
> document.
Oh, I assumed only hidden window can be eWindowType_invisible in bug 179056.
We will have to change the way if it is no loger true after widget removal.

Comment 12

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

It's not just after widget removal; since we started supporting external resource documents, we've had windows with eWindowType_invisible other than the hidden window.

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to comment #12)
> It's not just after widget removal; since we started supporting external
> resource documents, we've had windows with eWindowType_invisible other than the
> hidden window.
Thanks to the correction. I've filed bug 509023.

Comment 14

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

http://hg.mozilla.org/mozilla-central/rev/01bbe887b004

Comment 15

[Benjamin Smedberg]

Comment on attachment 393125 [details] [diff] [review]
fix

This is already a blocker.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

This was pushed to 1.9.2 at Tue Aug 25 12:00:46 2009 in changeset edcd7d79d61a:

http://hg.mozilla.org/releases/mozilla-1.9.2/pushloghtml?changeset=edcd7d79d61a

Fixed on 1.9.2.