833823 - Random, unpredictable behavior with multiple YouTube embeds

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[rkuchiki]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.52 Safari/537.17

Steps to reproduce:

Visit any page on WXVids.org that renders more than one YouTube element on a single page. For example, http://www.wxvids.org/archives/author/russell-hande

Other info:
Firefox 32-bit 18.0.1 Windows, running on 64-bit Windows 7.
Flash Player:
    File: NPSWF32_11_5_502_146.dll
    Version: 11.5.502.146
    Shockwave Flash 11.5 r502


Actual results:

It appears to be a random roulette. Sometimes they all load, most of the time some of them render a black element (but right clicking still gives you the YouTube player context menu).

On the elements that do load, the flash player may or may not operate correctly. The Play button may or may not work, and most of the time the status bar of the YouTube player is not accessible (such as volume, full screen, video quality).

This does not appear to happen if there is only 1 player on a page, and then Firefox behaves as desired.


Expected results:

All 5 video players should load and render correctly, without any issues.
When tested on the same machine with the following browsers, the desired effect occurs:

Internet Explorer 9 32-bit
Safari 5.1.7
Chrome 24.0.1312.52

Comment 1

[Chris Roeszler]

I have seen this same issue with multiple YouTube embedded vids in FireFox. I didn't test Safari, but IE and Chrome both worked as expected for me as well.

Comment 2

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/22288130fea2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120813211942
Bad:
http://hg.mozilla.org/mozilla-central/rev/d9183f015df8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120814055342
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=22288130fea2&tochange=d9183f015df8

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/303b75594832
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120813210542
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4ddd9c731adc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120813210943
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=303b75594832&tochange=4ddd9c731adc

Triggered by: Bug 775965

Comment 3

[Alice0775 White]

If the iframe is located outside of visible resion of current view, it seems to fail to load Flash content.

Comment 4

[Alex Keybl [:akeybl]]

roc - can you take a look? I would have sent this over to cpearce, but I understand he's in b2g-land. This wouldn't block FF19's release, but we would accept a low risk uplift up until this coming Tuesday (1/29).

Comment 5

[Alice0775 White]

In local build
Last Good: 9c75f0428f8a
First Bad: bcac58cbf328

Regressed by:
  bcac58cbf328	Chris Pearce — Bug 775965 - Ensure presentation persists across nsSubDocumentFrame reframes. r=roc

Comment 6

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

Attachment: Slightly more reduced testcase

This is a slightly more reduced testcase... I say "slightly", because the page invokes Googles Website Translator, which then runs a bunch of obfuscated JS. That's not the easiest thing to minimize...

Comment 7

[rkuchiki]

I've disabled the Translator widget, please advise if this helps, and when I can re--enable it.

Comment 8

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

Disabling the Translator widget means the bug goes away on your site, which will probably make your users happier! We have the reduced testcase now, so disabling the Translator widget on your page won't affect our ability to fix the bug.

Comment 9

[Alex Keybl [:akeybl]]

Chris - what are the next steps here? We'd like to resolve for FF20, considering this has missed FF19 and regressed in FF18.

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: patch

We reconstruct the whole document including iframes containing plugins without widgets. When we reconstruct we pick up the cached presentation and use nsObjectFrame::EndSwapDocShells to fix up the plugins. But we only register with the root prescontext if we have a widget. We need to do it even if we don't so that nsObjectFrame::DidSetWidgetGeometry gets called.

I just copied what we do in nsObjectFrame::PrepForDrawing, where we only don't register on mac when we don't have a widget.

Comment 11

[Timothy Nikkel (:tnikkel)]

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

Mats could review this too. Whoever gets to it first.

Comment 12

[Mats Palmgren (inactive)]

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

> I just copied what we do in nsObjectFrame::PrepForDrawing, where we only
> don't register on mac when we don't have a widget.

That seems reasonable, but it looks like EndSwapDocShells is a static method
so how does "if (mWidget)" compile?

Also, looking at nsObjectFrame::CallSetWindow there's this:

#ifdef XP_MACOSX
  nsWeakFrame weakFrame(this);
  mInstanceOwner->FixUpPluginWindow(nsPluginInstanceOwner::ePluginPaintDisable);
  if (!weakFrame.IsAlive()) {
    return NS_ERROR_NOT_AVAILABLE;
  }
#endif

so 'objectFrame' might be deleted in the code you added.

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren [:mats] from comment #12)
> That seems reasonable, but it looks like EndSwapDocShells is a static method
> so how does "if (mWidget)" compile?

I was testing on non-mac so I originally just made the call unconditional. When I went make the final patch I added in the mac case and assumed I had done it right, but I forgot the static part.

> Also, looking at nsObjectFrame::CallSetWindow there's this:
> 
> #ifdef XP_MACOSX
>   nsWeakFrame weakFrame(this);
>  
> mInstanceOwner->FixUpPluginWindow(nsPluginInstanceOwner::
> ePluginPaintDisable);
>   if (!weakFrame.IsAlive()) {
>     return NS_ERROR_NOT_AVAILABLE;
>   }
> #endif
> 
> so 'objectFrame' might be deleted in the code you added.

Good catch. This was a problem before my patch too. I'll just fix it though.

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 719577 [details] [diff] [review]
patch v2

Looks good, r=mats

Comment 16

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c21d0df646

Comment 17

[Timothy Nikkel (:tnikkel)]

Assuming we don't find any problems with this in a few days this should be safe enough for aurora and beta.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4c21d0df646

Comment 19

[Timothy Nikkel (:tnikkel)]

Comment on attachment 719577 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug  	775965
User impact if declined: plugins won't show in some circumstances
Testing completed (on m-c, etc.): been on mc for several days
Risk to taking this patch (and alternatives if risky): low risk fix
String or UUID changes made by this patch: none

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 719577 [details] [diff] [review]
patch v2

Low risk and early enough in FF20 beta, we can take this uplift.

Comment 21

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ecd6788ad15a
https://hg.mozilla.org/releases/mozilla-beta/rev/a3132e813f52

Comment 22

[Paul Silaghi, QA [:pauly]]

Verified fixed using the testcase on FF 20b3 on Win 7, Ubuntu 12.04 and Mac OS X 10.8.2

Comment 23

[u279076]

Flagging for verification in Firefox 21 and 22.

Henrik, do you think this could have a Mozmill testcase?

Comment 24

[Paul Silaghi, QA [:pauly]]

Verified fixed using the testcase on FF 21b3 on Win 7, Ubuntu 12.04 and Mac OS X 10.8.3

Comment 25

[Ioana (away)]

Verified as fixed on Firefox 22 beta 6 (20130617145905) on Windows 7 64bit, Ubuntu 12.10 64bit and Mac OSX 10.8.2.

Comment 26

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old

Comment 27

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

Removing my name from in-qa-testsuite flag for a better query.