Closed Bug 790265 Opened 12 years ago Closed 12 years ago

Click-to-play is broken on various websites like cnn.com or latimes.com

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Version:
15 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox17+ verified)

Status:
VERIFIED FIXED
Milestone:
mozilla18
Tracking Flags:
Tracking Status
firefox17 + verified

People

(Reporter: epinal99-bugzilla2, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 1 obsolete file)

testcase.html
477 bytes, text/html
Details
Changes that might attach a binding (e.g. state changes on <object> elements) should trigger a frame construction attempt even for display:none elements, so the binding will get attached.
4.93 KB, patch
dholbert
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
Click-to-play doesn't work on these websites with the latest Nightly:

1) http://edition.cnn.com/2012/06/25/health/georgia-flesh-eating-bacteria/index.html
Click on the video thumbnail ("click to play") in the article to make the video overlay appear.

2) http://games.latimes.com/games/daily-crossword/daily-crossword.aspx
Wait for the 30-sec commercial to see the puzzle.
Blocks: click-to-play
Might be a dupe of bug 787619, it looks click events are not making it to the CTP frame.
See Also: → 787619
Attached file testcase.html — 
My understanding is this occurs when a site embeds object content from another site, initially has "display: none", and then switches to something visible. This is very similar to bug 741130 but with the added wrinkle of coming from another domain. I think the fix is to again make sure the bindings get attached when creating the object in JS, but I don't know how to fix it myself.
Boris actually has a patch for this but we weren't sure which bug to put it in.
David, Blake, will we need backports of this to any branches?
Assignee: nobody → bzbarsky
Whiteboard: [need review]
It would be best if this were uplifted to 17 (Aurora, currently).
OK.  I personally think the patch is safe enough for that, but would welcome dbaron's opinion....
Comment on attachment 665505 [details] [diff] [review]
Changes that might attach a binding (e.g. state changes on <object> elements) should trigger a frame construction attempt even for display:none elements, so the binding will get attached.

Trying roc too, just in case.
Attachment #665505 - Flags: review?(roc)
Comment on attachment 665505 [details] [diff] [review]
Changes that might attach a binding (e.g. state changes on <object> elements) should trigger a frame construction attempt even for display:none elements, so the binding will get attached.

Actually, dholbert says he will.
Attachment #665505 - Flags: review?(roc)
Attachment #665505 - Flags: review?(dholbert)
Attachment #665505 - Flags: review?(dbaron)
Comment on attachment 665505 [details] [diff] [review]
Changes that might attach a binding (e.g. state changes on <object> elements) should trigger a frame construction attempt even for display:none elements, so the binding will get attached.

>diff --git a/content/xbl/test/test_bug790265.xhtml b/content/xbl/test/test_bug790265.xhtml
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=401907
>+-->
>+<head>
>+  <title>Test for Bug 401907</title>

4 instances of this ----^^^^^^ bug number that want to be "790265" instead.

>+    if (disp->mDisplay != NS_STYLE_DISPLAY_NONE ||
>+        // We should also try to recreate if we changed our binding
>+        // URI, because we may want to apply the new binding
>+        !EqualURIs(disp->mBinding, oldContext->PeekStyleDisplay()->mBinding)) {

Might be worth adding a brief parenthetical comment explaining why we know PeekStyleDisplay() will be non-null there (since in general, it can return null).

(or an assertion, which would simultaneously document & verify that assumption)

r=me either way, though.
Attachment #665505 - Flags: review?(dholbert) → review+
Comment on attachment 665505 [details] [diff] [review]
Changes that might attach a binding (e.g. state changes on <object> elements) should trigger a frame construction attempt even for display:none elements, so the binding will get attached.

>+    if (disp->mDisplay != NS_STYLE_DISPLAY_NONE ||
>+        // We should also try to recreate if we changed our binding
>+        // URI, because we may want to apply the new binding
>+        !EqualURIs(disp->mBinding, oldContext->PeekStyleDisplay()->mBinding)) {
>       result = RecreateFramesForContent(aElement, false);

Random drive-by comment after a quick glance:  is PeekStyleDisplay() guaranteed to return non-null?  Prefer either using GetStyleDisplay() or null-checking?
nsStyleContext::nsStyleContext calls ApplyStyleFixups() which calls GetStyleDisplay.

So I believe a style context will never return null from a PeekStyleDisplay call.

I guess I can add a null-check just in case that ever changes...  I'll go ahead and try to recreate if the new display has an mBinding and there is no old display struct.

Using GetStyleDisplay() is definitely wrong, I think, because we might not longer be able to compute style data for the old context.
Attachment #665505 - Attachment is obsolete: true
Comment on attachment 668706 [details] [diff] [review]
Changes that might attach a binding (e.g. state changes on <object> elements) should trigger a frame construction attempt even for display:none elements, so the binding will get attached.

Maybe we should just get rid of the 'result' variable, and directly return RecreateFramesForContent() and then stick a "return NS_OK;" at the end?

r=me either way, though.
Attachment #668706 - Flags: review?(dholbert) → review+
I went ahead and did that, and outdented things too, with early return if !oldContext.

https://hg.mozilla.org/integration/mozilla-inbound/rev/abf6515b8c85
Whiteboard: [need review]
Target Milestone: --- → mozilla18
Comment on attachment 668706 [details] [diff] [review]
Changes that might attach a binding (e.g. state changes on <object> elements) should trigger a frame construction attempt even for display:none elements, so the binding will get attached.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Click-to-play for plug-ins.
User impact if declined: Can't use click-to-play on some sites; clicking does
   nothing.
Testing completed (on m-c, etc.): Passes testcases in this bug.
Risk to taking this patch (and alternatives if risky): Low risk, I think.  Should
   just deoptimize a rare case slightly.
String or UUID changes made by this patch: None
Attachment #668706 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/abf6515b8c85
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 668706 [details] [diff] [review]
Changes that might attach a binding (e.g. state changes on <object> elements) should trigger a frame construction attempt even for display:none elements, so the binding will get attached.

[Triage Comment]
Carries non-zero risk, but the bug rears its head on some pretty major web properties. Approving for Aurora 17. Please land early Monday to make it in before the next merge.
Attachment #668706 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Boris, did you tested my 2 examples in comment #0 with your patch? Because the issue is not fixed on these 2 websites for me even if the testcase is fixed.
No, I had only tested the testcase, sorry.   Would you mind filing a second bug on those?  Looks like we need a better reduction of those sites.  :(
(In reply to Boris Zbarsky (:bz) from comment #21)
> Would you mind filing a second bug on those?

I'll.
Blocks: 800018
Testcase works fine after the fix. Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0b2
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: