637006 - Crash mainly close to startup @ QuickTimeWebHelper.qtx

Status: RESOLVED FIXED

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

Description

[Scoobidiver (away)]

It is a new crash signature that first appeared in 4.0b12pre/20110219.
It happens only at startup and is not specific to one version of QuickTime (7.6.6, 7.6.7, 7.6.8).
It is #110 top crasher in 4.0b13pre.

There are two kinds of stack traces:
47 	QuickTimeWebHelper.qtx 	QuickTimeWebHelper.qtx@0x1905a 	
48 	xul.dll 	mozilla::plugins::BrowserStreamChild::DeliverPendingData 	dom/plugins/BrowserStreamChild.cpp:287
49 	xul.dll 	mozilla::plugins::BrowserStreamChild::Deliver 	dom/plugins/BrowserStreamChild.cpp:230
50 	xul.dll 	MessageLoop::RunTask 	ipc/chromium/src/base/message_loop.cc:343
51 	xul.dll 	MessageLoop::DeferOrRunPendingTask 	ipc/chromium/src/base/message_loop.cc:351
52 	xul.dll 	MessageLoop::DoWork 	ipc/chromium/src/base/message_loop.cc:451
53 	xul.dll 	mozilla::layers::BasicShadowableImageLayer::~BasicShadowableImageLayer 	gfx/layers/basic/BasicLayers.cpp:1937
54 	xul.dll 	base::MessagePumpForUI::DoRunLoop 	ipc/chromium/src/base/message_pump_win.cc:213

2 	QuickTimeWebHelper.qtx 	QuickTimeWebHelper.qtx@0xea64 	
3 	xul.dll 	nsIDOMText_SplitText 	obj-firefox/js/src/xpconnect/src/dom_quickstubs.cpp:6625

More reports at:
https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=4&range_unit=weeks&signature=QuickTimeWebHelper.qtx%400xe4d9

Comment 1

[Benjamin Smedberg]

The stacks are partly bogus, but I think the DeliverPendingData may be correct. In any case, this is only a plugin-process crash, so I'm not that worried about it.

Comment 2

[Scoobidiver (away)]

I added the crash signature for QuickTime 7.6.9: QuickTimeWebHelper.qtx@0xe5d9.
It is currently #52 top crasher in 4.0b12.

Comment 3

[Marcia Knous [:marcia]]

In early RC crash data, this ranks #9 in the list of all crashes - http://tinyurl.com/4ksz67p. Will keep an eye on it.

Comment 4

[Scoobidiver (away)]

The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1eed87fac9ce&tochange=657c2a92ee2b
It could be a regression from bug 626016.

Comment 5

[Alice0775 White]

[str]
Open URL: http://sontag.ca/soundboards/Losers.html


Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/04527f9407f9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110217 Firefox/4.0b12pre ID:20110218130210
Fails:
http://hg.mozilla.org/mozilla-central/rev/990a62f9f3ce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110217 Firefox/4.0b12pre ID:20110218135548
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=04527f9407f9&tochange=990a62f9f3ce
regressed by:
990a62f9f3ce	Felipe Gomes — Bug 626016. Prevent QuickTime plugin from stealing focus when it's hidden due to SetWindow call. r=bsmedberg a=blocking-final

Comment 6

[Alice0775 White]

It works if i set dom.ipc.plugins.enabled to false.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

on it

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: reduced testcase

Thanks for finding a URL to reproduce this, Alice!

What happens (at least with this testcase) is that the way to determine if the plugin is hidden is different in nsObjectFrame and PluginInstanceChild.

nsObjectFrame::IsHidden() uses CSS's !IsVisibleOrCollapsed(), + the hidden attr for <embed> tags only (this is what facebook uses).
This info is not available to PluginInstanceChild, so the comparison there uses width == 0 && height == 0.

The testcase uses <object width="0" height="0">, so for all purposes nsObjectFrame consider it as _not_ hidden, however PluginInstanceChild blocks SetWindow calls on it.

This likely cause internal havoc to the quicktime plugin which then crashes. Without URLs in the crash reports it's hard to tell if all the crashes are caused by the same situation. 

Not sure yet the best place to propagate this info from the object frame down to plugin instance.

Comment 9

[Scoobidiver (away)]

Chris, can we get some URLs?

Comment 10

[chris hofmann]

we don't put url's in the plugin side off an OOPP report.   sometimes there are urls in the corresponding browser side stack, but for some reason all the links to the browser side stacks are coming up null (\N).   

So there is no way to trace back to urls that might be loaded when QuickTime crashes.

Comment 11

[Benjamin Smedberg]

This isn't a hang, so there are no browser-side reports... I believe that bug 643327 is a duplicate of this, and has a testcase which reproduces something like this.

Comment 13

[Benjamin Smedberg]

Felipe, we should really be using x/y/w/h==0, not just w/h, I think.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

I considered this in the original patch, but oddly enough, the x/y values (in the facebook case, at least) are not 0.

Comment 15

[Benjamin Smedberg]

That concerns me.

Comment 16

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #15)
> That concerns me.

Looked into these values and they're stable numbers that just reflect whatever position in the webpage that DOM element is inserted, even if it has 0 width and height. So I believe these numbers are fine (i.e. at least they're definitely not random uninitialized values). They just come from nsObjectFrame.mRect + the offset to the root frame.


The IsHidden return value on the other hand is used in various places, including view->SetViewVisibility, so it's easy to imagine this going wrong by not using it..

I think I should pass this down to the PluginInstanceChild. But when should I do it? SetViewVisibility is called from nsObjectFrame::DidReflow and DidSetStyleContext. Is it fine to add an IPC call in these functions? I fear that they might be called too often. (in this case I could cache the value and only call when the visibility status change)

Comment 17

[Jim Sontag]

Yesterday the following test page worked in FF 4 - today is does not. Yesterday the MP3 files all downloaded; today they stall until the mouse pointer moves over the control - and don't play back continuously, exactly the same behavior as I was having with FF 3.6.xx

http://sontag.ca/soundboards/Losers2.html

Also on this test page (with a 0x0 sized control) FF4 would give me a display simliar to this

https://bug643327.bugzilla.mozilla.org/attachment.cgi?id=521137

but today it's the same as I had with 3.6.xx. All controls load, no 'plug-n' required messages, and it does not work.

Can anyone suggest what might have happened?

Comment 18

[Jim Sontag]

(In reply to comment #17)

> Can anyone suggest what might have happened?

Oops - I had disabled all ipc controls.  I set all applicable ipc settings to TRUE, restarted Firefox, and it works with 1x1 controls, but not 0x0 controls.

A qtime crash occurs with 0x0 controls but not when the controls have non zero dimensions - see 

http://sontag.ca/soundboards/Losers.html
and
https://bug643327.bugzilla.mozilla.org/attachment.cgi?id=521137

This is unique to Firefox on Windows(?). IE8, Chrome, Safari and Opera all work with 0x0 controls.

I had originally disabled IPC on all 3 computers because I had runaway processor problems when the plug-in.container first appeared.  I'm assuming this problem has been solved. The auto upgrade of 3.6.15 to 4.0 appeared to honor the previous IPC settings I had.

If the IPC is disabled and Losers3 (with the visible controls) is loaded, the files stop downloading for many of the controls, but resume if you tickle the control by moving the mouse pointer over the respective control.

Comment 19

[Jim Sontag]

Another update - just tested a Win XP/FF 3.6.16 config with IPC controls set to all TRUE and 0x0 controls seem to work fine.

Comment 20

[Scoobidiver (away)]

I can reproduce it consistently at http://www.olnejean.net.

Comment 21

[Scoobidiver (away)]

It is #37 top crasher in 4.0.1.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Implemented as described in paragraph 3 from comment 16.

The bulk of this patch is passing the data from nsObjectFrame to PluginInstanceParent, which takes a lot of hops.

The idea is simple, whenever the value of IsHidden() changes and updates the view manager, we pass this information down to the plugin.. Then it can use accurate info to determine if it's visible or not (only used in the quicktime quirk check).

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

(sorry, attached wrong version of the patch)

Comment 24

[Benjamin Smedberg]

Comment on attachment 532092 [details] [diff] [review]
Patch

I really don't understand why we need a separate call: the NPAPI says that setwindow(0,0,0,0) is the signal to all plugins that they are invisible. I still don't understand why we need this separate signal.

And roc should probably review this.

Comment 25

[Benjamin Smedberg]

Being a plugin crash, I don't think we need to track this for a particular release.

Comment 26

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #24)
> I really don't understand why we need a separate call: the NPAPI says that
> setwindow(0,0,0,0) is the signal to all plugins that they are invisible. I
> still don't understand why we need this separate signal.

Ok, so it seems that there are two different things here:

1 - at least for some cases, an invisible plugin is not receiving setwindow(0,0,0,0) calls (that is the case with the plugin in FB and in the reduced testcase too)

2 - should a <object width="0" height="0"> plugin be considered invisible, and thus receive setwindow(0,0,0,0)? Or should that happen only for <object hidden="true"> and elements hidden with css display:none and visibility:collapse?


I'm trying to answer why #1 happens

Comment 27

[Benjamin Smedberg]

The NPAPI spec doesn't actually say, but I believe #2 should also receive 0,0,0,0, because it is in fact not visible at all.

Comment 28

[Scoobidiver (away)]

Currently #3 top crasher in 5.0b5.

Comment 29

[Robert Kaiser]

(In reply to comment #28)
> Currently #3 top crasher in 5.0b5.

That is if you exclude hangs but include plugin crashes. In total reports, it's #10, in plugin-only issues it's #4, in the usual browser crashes list, it doesn't exist at all, as it's a plugin crash.

Comment 31

[Marcia Knous [:marcia]]

2,661 Crash Reports per week in this signature across all Firefox versions.

Comment 32

[Scoobidiver (away)]

With combined signatures, it is #67 top crasher and #14 top plugin crasher in 7.0.1.
Can someone review the patch?

Comment 35

[Robert Kaiser]

https://crash-stats.mozilla.com/report/list?signature=quicktimewebhelper.qtx%400xebf9 is #27 on 9.* yesterday. As there is a potential patch around, I'm nominating this one.

Felipe, did you look into this any more?

Comment 36

[:Felipe Gomes (needinfo for replies!)]

> Felipe, did you look into this any more?

I tried to implement the suggestion from comment 24 some time ago but it was making other related tests fail. I will take a look at this again this week but if someone else who understands more of the plugin visibility behavior wants to take the bug they should.

The existing patch is very bitrotted since it uses some structures that were refactored after the patch was written. Also it was said to not be the right approach, although it might be the approach required to deal with all the plugins' quirks around visibility.

Comment 37

[Scoobidiver (away)]

It's #2 top plugin crasher in 10.0.

Comment 38

[Robert Kaiser]

(In reply to Scoobidiver from comment #37)
> It's #2 top plugin crasher in 10.0.

How did you find that? I don't find QuickTimeWebHelper.qtx crashes anywhere recently.

Comment 39

[siteturbo]

Basically what happens is when opening a PDF document the browser hangs and the spinner/hourglass just turns and turns.  You can't do anything.  The only thing you can do is close the browser and restart firefox. Then it works fine again.

Comment 40

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) from comment #38)
> How did you find that? I don't find QuickTimeWebHelper.qtx crashes anywhere
> recently.
You probably let the Browser filter in top crashers. See https://crash-stats.mozilla.com/topcrasher/byversion/Firefox/10.0.1/7/plugin
It was #31 top plugin crasher in 9.0.1 over the last month.

According to bug 716945 and bug 722394, I think it isn't worth adding the topcrash keyword for plugin crashes and hangs in 10.0, 10.0.1, 11.0b1, and 11.0b2, so I removed it.

Comment 41

[Jim Sontag]

Humble apologies. I know how tough this job can be. My last comment on this thread (#19) still stands. A bug in a 3.6.17(?) release has still not been resolved. A bug that should be easy to fix.

A server can serve up an object with a 0x0 dimension. It is not rendered but it is there.

All other browsers I have tested have NO PROBLEM with this, only FF.  My current FF version is 10.0.1.

For a demo, try out: http://sontag.ca/soundboards/Losers.html

I run on a WAMP stack - please don't hold that against me ;-)

Comment 42

[Robert Kaiser]

(In reply to Scoobidiver from comment #40)
> (In reply to Robert Kaiser (:[e-mail]) from comment #38)
> > How did you find that? I don't find QuickTimeWebHelper.qtx crashes anywhere
> > recently.
> You probably let the Browser filter in top crashers.

No, I was doing a search - but for the version with capital letters in it, while only the fully-lowercase version can be found nowadays.

Comment 43

[Sheila Mooney]

I don't see this in the top 300 overall but I still think we should try and fix it since it's at #3 on the plugin list. Felipe, if you don't have time to look into it, is there someone else we can assign this to. I am removing the tracking flag for now.

Comment 44

[Bob Clary [:bc] (inactive)]

I started testing QuickTime on Windows in crash automation and hit this across all branches.

Comment 45

[:Felipe Gomes (needinfo for replies!)]

I think Josh would be able to look at this much better than me at this time, specially considering the changes introduced with bug 90268. I tried implementing the suggestion in comment 24 here but it made test_xulbrowser_plugin_visibility.xul to fail.
Alternatively we could think about backing out the original bug 626016 and using the alternative fix proposed in bug 629593 (but at the time bsmedberg said he preferred the fix in 626016).

Comment 46

[Alice0775 White]

affected  http://www.patsyclinetributeshow.com

Comment 47

[Carsten Book [:Tomcat]]

can't reproduce this anymore so marking as wfm for now - feel free to reopen if we have a new testcase or so