Last Comment Bug 833823 - Random, unpredictable behavior with multiple YouTube embeds
: Random, unpredictable behavior with multiple YouTube embeds
Status: RESOLVED FIXED
: regression, reproducible
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 17 Branch
: x86_64 All
: -- normal (vote)
: mozilla22
Assigned To: Timothy Nikkel (:tnikkel)
:
Mentors:
Depends on:
Blocks: 775965
  Show dependency treegraph
 
Reported: 2013-01-23 08:23 PST by rkuchiki
Modified: 2014-08-29 15:24 PDT (History)
13 users (show)
anthony.s.hughes: in‑qa‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
affected
+
wontfix
+
verified
+
verified
verified
-


Attachments
Slightly more reduced testcase (1.58 KB, text/html)
2013-01-29 19:12 PST, Chris Pearce (:cpearce)
no flags Details
patch (1.08 KB, patch)
2013-02-27 19:02 PST, Timothy Nikkel (:tnikkel)
mats: review-
Details | Diff | Splinter Review
patch v2 (1.27 KB, patch)
2013-02-28 10:38 PST, Timothy Nikkel (:tnikkel)
mats: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description rkuchiki 2013-01-23 08:23:31 PST
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 2013-01-23 08:40:29 PST
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 2013-01-23 09:14:32 PST
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 2013-01-23 10:46:39 PST
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] 2013-01-23 12:28:07 PST
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 2013-01-26 03:47:52 PST
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) 2013-01-29 19:12:32 PST
Created attachment 707962 [details]
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 2013-01-29 19:36:25 PST
I've disabled the Translator widget, please advise if this helps, and when I can re--enable it.
Comment 8 Chris Pearce (:cpearce) 2013-01-30 18:46:20 PST
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] 2013-02-07 09:49:26 PST
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) 2013-02-27 19:02:05 PST
Created attachment 719288 [details] [diff] [review]
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) 2013-02-28 08:00:12 PST
Comment on attachment 719288 [details] [diff] [review]
patch

Mats could review this too. Whoever gets to it first.
Comment 12 Mats Palmgren (:mats) 2013-02-28 10:08:39 PST
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) 2013-02-28 10:37:28 PST
(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) 2013-02-28 10:38:20 PST
Created attachment 719577 [details] [diff] [review]
patch v2
Comment 15 Mats Palmgren (:mats) 2013-02-28 10:49:29 PST
Comment on attachment 719577 [details] [diff] [review]
patch v2

Looks good, r=mats
Comment 16 Timothy Nikkel (:tnikkel) 2013-02-28 11:17:01 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4c21d0df646
Comment 17 Timothy Nikkel (:tnikkel) 2013-02-28 11:17:39 PST
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] 2013-02-28 19:44:38 PST
https://hg.mozilla.org/mozilla-central/rev/d4c21d0df646
Comment 19 Timothy Nikkel (:tnikkel) 2013-03-03 16:18:56 PST
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 2013-03-04 13:11:26 PST
Comment on attachment 719577 [details] [diff] [review]
patch v2

Low risk and early enough in FF20 beta, we can take this uplift.
Comment 22 Paul Silaghi, QA [:pauly] 2013-03-06 01:08:07 PST
Verified fixed using the testcase on FF 20b3 on Win 7, Ubuntu 12.04 and Mac OS X 10.8.2
Comment 23 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-03-08 15:19:35 PST
Flagging for verification in Firefox 21 and 22.

Henrik, do you think this could have a Mozmill testcase?
Comment 24 Paul Silaghi, QA [:pauly] 2013-04-17 05:55:40 PDT
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) 2013-06-18 05:16:15 PDT
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] 2014-01-10 10:41:53 PST
mass remove verifyme requests greater than 4 months old
Comment 27 Henrik Skupin (:whimboo) 2014-08-29 15:24:17 PDT
Removing my name from in-qa-testsuite flag for a better query.

Note You need to log in before you can comment on or make changes to this bug.