Closed Bug 833823 Opened 7 years ago Closed 7 years ago

Random, unpredictable behavior with multiple YouTube embeds

Categories

(Core :: Layout, defect)

17 Branch
x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox18 - affected
firefox19 + wontfix
firefox20 + verified
firefox21 + verified
firefox22 --- verified
firefox-esr17 - ---

People

(Reporter: rkuchiki, Assigned: tnikkel)

References

Details

(Keywords: regression, reproducible)

Attachments

(2 files, 1 obsolete file)

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
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.
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
Blocks: 775965
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Version: 18 Branch → 17 Branch
OS: Windows 7 → All
If the iframe is located outside of visible resion of current view, it seems to fail to load Flash content.
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).
In local build
Last Good: 9c75f0428f8a
First Bad: bcac58cbf328

Regressed by:
  bcac58cbf328	Chris Pearce — Bug 775965 - Ensure presentation persists across nsSubDocumentFrame reframes. r=roc
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...
I've disabled the Translator widget, please advise if this helps, and when I can re--enable it.
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.
Chris - what are the next steps here? We'd like to resolve for FF20, considering this has missed FF19 and regressed in FF18.
Assignee: cpearce → ajones
Assignee: ajones → tnikkel
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #719288 - Flags: review?(roc)
Comment on attachment 719288 [details] [diff] [review]
patch

Mats could review this too. Whoever gets to it first.
Attachment #719288 - Flags: review?(matspal)
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.
Attachment #719288 - Flags: review?(matspal) → review-
Attachment #719288 - Flags: review?(roc)
(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.
Attached patch patch v2Splinter Review
Attachment #719288 - Attachment is obsolete: true
Attachment #719577 - Flags: review?(matspal)
Comment on attachment 719577 [details] [diff] [review]
patch v2

Looks good, r=mats
Attachment #719577 - Flags: review?(matspal) → review+
Assuming we don't find any problems with this in a few days this should be safe enough for aurora and beta.
https://hg.mozilla.org/mozilla-central/rev/d4c21d0df646
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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
Attachment #719577 - Flags: approval-mozilla-beta?
Attachment #719577 - Flags: approval-mozilla-aurora?
Comment on attachment 719577 [details] [diff] [review]
patch v2

Low risk and early enough in FF20 beta, we can take this uplift.
Attachment #719577 - Flags: approval-mozilla-beta?
Attachment #719577 - Flags: approval-mozilla-beta+
Attachment #719577 - Flags: approval-mozilla-aurora?
Attachment #719577 - Flags: approval-mozilla-aurora+
Verified fixed using the testcase on FF 20b3 on Win 7, Ubuntu 12.04 and Mac OS X 10.8.2
Flagging for verification in Firefox 21 and 22.

Henrik, do you think this could have a Mozmill testcase?
Flags: in-qa-testsuite?(hskupin)
Keywords: verifyme
Verified fixed using the testcase on FF 21b3 on Win 7, Ubuntu 12.04 and Mac OS X 10.8.3
Verified as fixed on Firefox 22 beta 6 (20130617145905) on Windows 7 64bit, Ubuntu 12.10 64bit and Mac OSX 10.8.2.
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Removing my name from in-qa-testsuite flag for a better query.
Flags: in-qa-testsuite?(hskupin) → in-qa-testsuite?
You need to log in before you can comment on or make changes to this bug.