Loic Reporter
	Description • 12 years ago

+++ This bug was initially created as a clone of Bug #790265 +++

Click-to-play doesn't work on these websites with the latest Nightly and the patch landed in bug 790265 doesn't fix the issue (even if the testcase attached is fixed).

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.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 12 years ago

Need better reductions of those sites...

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 2 • 12 years ago

From my understanding, all of these problems are from the plugin overlay bindings not being attached to the plugin object at this point:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-plugins.js#118

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 12 years ago

Yes, but the question is _why_?

There are lots of things that can cause a binding not to be attached.  The question is which of them are relevant here.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 4 • 12 years ago

I guess what I meant is, if we're at this point in the code, the bindings need to be attached. If there were some way of guaranteeing that, that would be great. (The flip-side is there might be occasions where we shouldn't be running this code (bug 797945, possibly), but that's not the case here.)

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 12 years ago

It's hard to see why they're not attached without understanding what the site is doing....

	u279076
	Comment 6 • 12 years ago

WFM but only after I've done a blocklist ping. Before pinging the blocklist I get an infobar block, after the ping I get a CTP block. Tested with Youtube.com, CNN.com, and LATimes.com; Firefox 17.0b1, 18.0a2, and 19.0a1 on Windows 7 SP1.

Note that the blocklist ping is not immediate on new profiles. This is expected AFAIK.

	u279076
	Comment 7 • 12 years ago

Forgot to mention, I tested with Flash 10.2.153.1 (Flash 10.3 and newer are not currently blocked).

	Paul Silaghi, QA [:pauly]
	Comment 8 • 12 years ago

I confirm the STR in comment 0 on Nightly 19.0a1 (2012-10-18) with plugins.click_to_play pref set on TRUE. Removing qawanted for now.

	Boris Zbarsky [:bzbarsky]
	Comment 9 • 12 years ago

The qawanted is there because we need a smaller testcase, not because we're questioning whether the bug is present.

	u279076
	Comment 10 • 12 years ago

Paul since you are able to reproduce this, can you please try to come up with a minimized testcase which can be attached to this bug based on the STR in comment 0?

	u279076
	Comment 11 • 12 years ago

QA is a bit distracted this week with multiple releases and trying to test the staged CTP block for Flash and Java. Can we get some help from Engineering to narrow down the testcase? Otherwise this will likely need to wait until next week.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 12 • 12 years ago

KFT, can you make a reduced testcase here?

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 13 • 12 years ago

With the 1 second delay it shows "Click here to activate plugin" rather than "Click here to activate Adobe Flash plugin" without the delay.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 14 • 12 years ago

Loading that testcase from Bugzilla shows me nothing at all, just a blank page. Does it work for you?

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 15 • 12 years ago

Did you set (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Loading that testcase from Bugzilla shows me nothing at all, just a blank
> page. Does it work for you?

Yes. Did you set plugins.click_to_play to true?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 16 • 12 years ago

(In reply to Anthony Jones (:kentuckyfriedtakahe) from comment #15)
> Yes. Did you set plugins.click_to_play to true?

No. That was it.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 17 • 12 years ago

Thanks Anthony! You did most of the hard work, but I reduced it even more to make it clearer what the problem is.

Dynamically adding an <object> to a display:none parent seems to be the important step here. Then when we remove the display:none after a delay, things don't work. If I put the <object> directly in the DOM under the display:none parent, things work fine.

I have no idea how CTP actually works so I can't offer any more detailed hypotheses.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 12 years ago

CTP depends on the XBL binding being attached before it fires its events.

The binding is attached (or not) based on state changes on the <object>, iirc.

Basically, CTP wants bindings to be attached independent of frame state, which is just not how things work in Gecko...

There are various hacks around that including the fix for bug 790265 and the fix for bug 741130.

But if we instantiate plug-ins in display:none subtrees, or at least fire the relevant CTP events (which this testcase sure sounds like we do) shouldn't the _real_ fix just be for the object code that fires the event the CTP stuff triggers from to just make sure the binding is attached instead of trying to rely on invariants that simply don't hold?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 19 • 12 years ago

(In reply to Boris Zbarsky (:bz) from comment #18)
> But if we instantiate plug-ins in display:none subtrees,

We don't, in general. Web compatibility requires we don't.

> or at least fire
> the relevant CTP events (which this testcase sure sounds like we do
> shouldn't the _real_ fix just be for the object code that fires the event
> the CTP stuff triggers from to just make sure the binding is attached
> instead of trying to rely on invariants that simply don't hold?

I think in this case we should be firing the CTP events when the plugin comes out of display:none.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 20 • 12 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19)
> (In reply to Boris Zbarsky (:bz) from comment #18)
> > But if we instantiate plug-ins in display:none subtrees,
> 
> We don't, in general. Web compatibility requires we don't.

What about, for instance, accessing a plugin from a script on a page? I'm pretty sure we at least try to instantiate the plugin then, in which case the click-to-play event will be fired.

(In reply to Boris Zbarsky (:bz) from comment #18)
> shouldn't the _real_ fix just be for the object code that fires the event
> the CTP stuff triggers from to just make sure the binding is attached
> instead of trying to rely on invariants that simply don't hold?

That would be awesome.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 12 years ago

> What about, for instance, accessing a plugin from a script on a page?

Then, in this particular case where it has a wrapper already, we won't hit the classinfo code...

> in which case the click-to-play event will be fired.

And at that point we should attempt to attach the binding.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 12 years ago

(In reply to David Keeler from comment #20)
> What about, for instance, accessing a plugin from a script on a page? I'm
> pretty sure we at least try to instantiate the plugin then, in which case
> the click-to-play event will be fired.

Yes.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 23 • 12 years ago

I don't think Anthony is the right assignee here anymore. Guessing it might be John.

	John Schoenick [:johns] Assignee
	Comment 24 • 12 years ago

I should have a patch shortly which should be pretty low risk for 17,

(In reply to David Keeler from comment #20)
> What about, for instance, accessing a plugin from a script on a page? I'm
> pretty sure we at least try to instantiate the plugin then, in which case
> the click-to-play event will be fired.

We try, but fail, to instantiate the plugin in this case. Should we fire a CTP event and then a second event when a frame exists?

And if a frame is thrown out then re-attached, will re-firing the event cause issues for the front end?

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 25 • 12 years ago

(In reply to John Schoenick [:johns] from comment #24)
> I should have a patch shortly which should be pretty low risk for 17,
> 
> (In reply to David Keeler from comment #20)
> > What about, for instance, accessing a plugin from a script on a page? I'm
> > pretty sure we at least try to instantiate the plugin then, in which case
> > the click-to-play event will be fired.
> 
> We try, but fail, to instantiate the plugin in this case. Should we fire a
> CTP event and then a second event when a frame exists?
> 
> And if a frame is thrown out then re-attached, will re-firing the event
> cause issues for the front end?

That might work. The only issue I anticipate is the front-end will have to be a little more robust in terms of checking for the bindings and safely stopping in case they aren't there. It would probably also be a good idea to make certain that any actions the front-end takes are idempotent.

	John Schoenick [:johns] Assignee
	Comment 26 • 12 years ago

So I ran into a significant issue with this -

As far as I'm aware, content has no good way of being notified when a binding is attached -- ObjLC's HasNewFrame is only for object frames, not the fallback frames that these events are for.

The proper way to fix this would be to make the binding itself in charge of firing the event when it is created, calling nsIObjectLoadingContent.GetPluginFallbackType() to get the appropriate info. This is a very significant change, however, and probably wouldn't be suitable for 17.

I'm looking to see if I can come up with a wallpaper-y fix that would be easier for branches

	John Schoenick [:johns] Assignee
	Comment 27 • 12 years ago

dkeeler is looking into a patch to have the bindings fire their own events, this
patch removes them from ObjLC. This doesn't cover plugin crashed events, which
I'll open a followup for

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 12 years ago

> content has no good way of being notified when a binding is attached 

When a binding is attached, or when a frame is created?  We could easily add a notification when a binding is attached...

> The proper way to fix this would be to make the binding itself in charge of firing the
> event when it is created

Via the constructor, yes.  If this is an option it's definitely most preferable.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 29 • 12 years ago

Unfortunately, it doesn't look like this is as simple as we'd like it to be. Since the overlay binding is in-page, it doesn't have permission to access gBrowser to dispatch plugin click-to-play/etc. events to it.

I also tried having nsObjectLoadingContent observe an event I added nsXBLService when it loads a binding, but I'm having trouble getting that working.

Any pointers on the right direction to go here?

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 12 years ago

The binding could fire untrusted events and the chrome could look for them and verify the target really does have the binding....

	John Schoenick [:johns] Assignee
	Comment 32 • 12 years ago

Feedback: dkeeler@mozilla.com

	John Schoenick [:johns] Assignee
	Comment 33 • 12 years ago

Comment on attachment 678555 [details] [diff] [review]
Part 2 - Have the pluginProblem binding fire an event when bound, then use objlc.pluginFallbackType to determine what handler to use

Apparently git-bz doesn't handle the Feedback: keyword...

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 34 • 12 years ago

Comment on attachment 678555 [details] [diff] [review]
Part 2 - Have the pluginProblem binding fire an event when bound, then use objlc.pluginFallbackType to determine what handler to use

Review of attachment 678555 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. There are some other changes that should be made (there are some cases where we know the binding is attached, so we don't have to test for it or try to get it to attach), but I can take care of that.
Also, I'll have a more thorough look tomorrow, but it does look like nothing will go wrong if the event handling code is called multiple times (with maybe the exception of the link handlers).
Also also, I think we have to do the same thing to fennec's browser.js.

::: browser/base/content/browser-plugins.js
@@ +138,5 @@
> +        // Not all states map to a handler
> +        return;
> +    }
> +  },
> +  

Looks like some whitespace here.

@@ +149,5 @@
>      if (!(plugin instanceof Ci.nsIObjectLoadingContent))
>        return;
>  
>      // Force a style flush, so that we ensure our binding is attached.
>      plugin.clientTop;

We don't need to do this anymore, since we know the binding is attached (or, rather, we're checking in a few lines).

@@ +161,5 @@
> +      if (!overlay || overlay._bindingHandled) {
> +        return;
> +      }
> +      overlay._bindingHandled = true;
> +      

Some more whitespace.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 36 • 12 years ago

Margaret - we're making some changes to the plugin click-to-play event structure, so we have to change fennec's browser.js. Would you mind taking a look?
(There are a lot of whitespace only changes, so I'll post a 'diff -w' version that's easier to read.)

	John Schoenick [:johns] Assignee
	Comment 38 • 12 years ago

Addressed dkeeler's feedback

	John Schoenick [:johns] Assignee
	Comment 39 • 12 years ago

I raced with david fixing fennec's browser.js, but here's the of-questionable-worth android xul fixes

	John Schoenick [:johns] Assignee
	Comment 40 • 12 years ago

Comment on attachment 677912 [details] [diff] [review]
Part 1 - Remove binding events from nsObjectLoadingContent

@Josh - the idea here is we're removing most of these events, and having the binding fire a "PluginBindingAttached" when it is created, which solves a few issues with firing events before bindings exist, as well as fixing bindings that arn't attached until a later date.

	John Schoenick [:johns] Assignee
	Comment 41 • 12 years ago

Comment on attachment 678903 [details] [diff] [review]
Part 2 - Have the pluginProblem binding fire an event when bound, then use objlc.pluginFallbackType to determine what handler to use

See comment 40 for what's going on here

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 42 • 12 years ago

Comment on attachment 678906 [details] [diff] [review]
Part 5ish - Fix android xul as well

Review of attachment 678906 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/xul/chrome/content/content.js
@@ +1626,3 @@
>      let plugin = event.target;
> +    let doc = event.ownerDocument;
> +    if (!doc || !(plugin instanceof Ci.nsIObjectLoadingContent)))

Do we need to test for the existence of doc elsewhere, as well? (I'm thinking in android fennec's browser.js...)

@@ +1634,5 @@
> +    }
> +    overlay._bindingHandled = true;
> +
> +    // Lookup the handler for this binding
> +    eventType = self._getBindingType(plugin);

Does this refer to this the _getBindingType that's in browser-plugins.js? (Otherwise, does this work? I don't think this function is defined here...)

	John Schoenick [:johns] Assignee
	Comment 43 • 12 years ago

Folded parts two and three

	John Schoenick [:johns] Assignee
	Comment 44 • 12 years ago

margaret, are you the right person to look at the android xul changes as well? (they're relatively straight-forward)

	John Schoenick [:johns] Assignee
	Comment 45 • 12 years ago

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=80a61fd67ccd

A few plugin-fallback tests burned, but these look to be due to the test assuming the event fires instantly, instead of after a layout flush.

	Mozilla RelEng Bot
	Comment 46 • 12 years ago

Try run for 80a61fd67ccd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=80a61fd67ccd
Results (out of 99 total builds):
    success: 66
    warnings: 33
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jschoenick@mozilla.com-80a61fd67ccd

	Ioana (away)
	Comment 47 • 12 years ago

Firefox 17 beta 5 Flash 11.2

STR:
1. Activate click to play and restart Firefox.
2. Load http://www.sohu.com/ in the browser.
3. Click on all the gray areas with "Click here to activate the Adobe Flash plugin" on them.

Actual Results:
The plugin content gets loaded on the first few containers at the beginning of the page. When clicking on those from the bottom of the page, a link is opened in a new tab.

Could this issue be related to the one tracked here?

	Georg Fritzsche [:gfritzsche]
	Comment 48 • 12 years ago

(In reply to Ioana Budnar [QA] from comment #47)
> Could this issue be related to the one tracked here?

No, it's unrelated. Those are ad-links which are z-index-layered over the Flash. The rest of the ads are setting up Flash 'clickthru' instead.

	Alex Keybl [:akeybl]
	Comment 49 • 12 years ago

We're leaning towards landing this fix on Nightly/Aurora asap, Beta before Monday, and then deciding whether or not to use CTP blocklisting depending on any regressions found before/after release.

If we think it's probable that we'll find a worse new regression than we've fixed, we should consider whitelisting cnn.com and latimes.com for FF17.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 50 • 12 years ago

Comment on attachment 678915 [details] [diff] [review]
Part 2 - Have the pluginProblem binding fire an event when bound, then use objlc.pluginFallbackType to determine what handler to use

Review of attachment 678915 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the below changes.

::: browser/base/content/browser-plugins.js
@@ +234,2 @@
>        let overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> +      if (self.isTooSmall(plugin, overlay))

I'd prefer to keep the |overlay != null| check here for safety.

@@ +358,5 @@
>        let objLoadingContent = aPlugin.QueryInterface(Ci.nsIObjectLoadingContent);
>        objLoadingContent.playPlugin();
>        return;
>      } else if (pluginsPermission == Ci.nsIPermissionManager.DENY_ACTION) {
> +      overlay.style.visibility = "hidden";

Same thing here and immediately below, keep the |if (overlay)|.

::: browser/base/content/test/browser_pluginplaypreview.js
@@ +160,5 @@
>  }
>  
> +function handleBindingAttached(evt) {
> +  evt.target instanceof Ci.nsIObjectLoadingContent;
> +  if (evt.target.pluginFallbackType == Ci.nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW)

The first line here doesn't seem like it is doing what you wanted. It looks to be a no-op right now. I think you meant,
> if (evt.target instanceof Ci.nsIObjectLoadingContent &&
>     evt.target.pluginFallbackType == Ci.nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW)

	:Margaret Leibovic
	Comment 51 • 12 years ago

Comment on attachment 678918 [details] [diff] [review]
Part 4 - Fix android xul as well

Did you test this? Does it even work? I ask because hg blame tells me this was added by blassey long ago the first time we tried to make click to play plugins [1], and a lot has changed since then.

As long as this doesn't totally bust xul fennec, we can take it, but I don't know if it's worth trying to patch xul fennec.

[1] http://hg.mozilla.org/mozilla-central/rev/658653c5836d

	:Margaret Leibovic
	Comment 52 • 12 years ago

Comment on attachment 678899 [details] [diff] [review]
part 3 - fennec browser.js changes

Review of attachment 678899 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. My one concern is that handleEvent() is getting pretty huge, and I think it would be more appropriate to move this plugin logic to PluginHelper, rather than continue to keep it in Tab. This is a pre-existing issue, so it won't stop me from giving an r+, but I'll be quick to review an updated patch if you agree :)

::: mobile/android/chrome/content/browser.js
@@ +2653,5 @@
>      this.browser.removeEventListener("DOMWindowClose", this, true);
>      this.browser.removeEventListener("DOMWillOpenModalDialog", this, true);
>      this.browser.removeEventListener("scroll", this, true);
>      this.browser.removeEventListener("MozScrolledAreaChanged", this, true);
> +    this.browser.removeEventListener("PluginBindingAttached", this, true, true);

removeEventListener doesn't take a fourth parameter, so you can get rid of that.
https://developer.mozilla.org/en-US/docs/DOM/element.removeEventListener

@@ +3001,5 @@
>        this.setDisplayPort(displayPort);
>    },
>  
> +  // Helper to get the binding handler type from a plugin object
> +  _getBindingType : function(plugin) {

Nit: Don't put a space between the property name and the function declaration.

@@ +3188,5 @@
>          this.sendViewportUpdate(true);
>          break;
>        }
>  
> +      case "PluginBindingAttached": {

To avoid confusion between this top-level switch statement and the switch statement you create below, could we factor out the code in here into a separate function? Maybe it would work well to put that function in PluginHelper, something like handlePluginBindingAttached(eventType).

@@ +3232,5 @@
> +            }
> +
> +            // Add click to play listener to the overlay
> +            overlay.addEventListener("click", function(e) {
> +              if (e) {

I realize this code was already in here, but I don't think we need this check. I think e will always be a valid event, and we use it without a check down below anyway. Seems like we can assume that e exists, then just bail if (!e.isTrusted). But we can fix this elsewhere to minimize risk if this patch needs to be uplifted.

	John Schoenick [:johns] Assignee
	Comment 53 • 12 years ago

Comment on attachment 678918 [details] [diff] [review]
Part 4 - Fix android xul as well

(In reply to Margaret Leibovic [:margaret] from comment #51)
> Comment on attachment 678918 [details] [diff] [review]
> Part 4 - Fix android xul as well
> 
> Did you test this? Does it even work?

I've no way to test XUL fennec, but this is identical code to the native fennec changes, re-routing one event. I'm fine with not bothering with the patch though - looking at the code I'm inclined to agree that it's already busted.

	John Schoenick [:johns] Assignee
	Comment 55 • 12 years ago

Comment on attachment 679492 [details] [diff] [review]
More test fixups

This fixes the remaining broken tests, which were mostly just using the obsolete event.

There is one possible issue here, though - previously, we would show a missing plugin banner for <object type="some/missingtype"></object>, but not a error frame, due to bug 548133. Now, since the error frame triggers the event, that object would not have a banner. The patch on 548133 removes this special casing, so we should just land on top of that.

If we do decide to take this in beta 6, I can revert a special case of the part 1 patch to still fire these events for the "pluginurl" param (but I don't think trying to land this on beta is advisable anyway)

	Michael Coates [:mcoates] (acct no longer active)
	Comment 56 • 12 years ago

Perhaps not related to this root cause, but pandora.com doesn't work either. You have to click on the "click to play" icon in the url bar to allow flash on the site. Otherwise the site never loads a song.

	Andrew McCreight [:mccr8]
	Comment 57 • 12 years ago

(In reply to Michael Coates [:mcoates] from comment #56)
> Perhaps not related to this root cause, but pandora.com doesn't work either.
> You have to click on the "click to play" icon in the url bar to allow flash
> on the site. Otherwise the site never loads a song.

Pandora uses Flash to play the music, but it does not actually display anything visible for Flash, so there's just nothing to click on.

	Paul Silaghi, QA [:pauly]
	Comment 58 • 12 years ago

(In reply to Andrew McCreight [:mccr8] from comment #57)
> (In reply to Michael Coates [:mcoates] from comment #56)
> > Perhaps not related to this root cause, but pandora.com doesn't work either.
> > You have to click on the "click to play" icon in the url bar to allow flash
> > on the site. Otherwise the site never loads a song.
> 
> Pandora uses Flash to play the music, but it does not actually display
> anything visible for Flash, so there's just nothing to click on.

bug 809846

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 59 • 12 years ago

I forgot to bring this up in the meeting (sorry about that), but I would like this patch to be considered as a slight amelioration of the worst case of this bug. In "non-blocklisted" click-to-play, users can still use the popup notification to activate plugins if they encounter this bug. However, if a plugin is "vulnerable/updatable" click-to-play blocklisted, there's some extra work the front-end does that assumes the existence of the binding. Since it isn't attached, the code bails before it can set up the popup. So, there is no way for a user to work around this bug in that case. Since we're not going to be blocking flash in this way, there probably won't be anyone who encounters this, but if someone does (say with java or silverlight), it would be nice to have the popup be available as a workaround.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play/blocklisting plugins
User impact if declined: see above
Testing completed (on m-c, etc.): https://tbpl.mozilla.org/?tree=Try&rev=e42b2d03f777
Risk to taking this patch (and alternatives if risky): very low - it's just some null checks in js and a small test
String or UUID changes made by this patch: none

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 60 • 12 years ago

Comment on attachment 679904 [details] [diff] [review]
beta workaround

Given that we're not blocklisting Flash for 17 and we don't think users will hit this with Java/Silverlight plugins, let's stick with no more landings on beta that are not critical at this point.

	Daniel Veditz [:dveditz]
	Comment 61 • 12 years ago

Another broken site is https://www.humblebundle.com/ which is currently offering Android games. If you click on one of the game icons on the phone image you get an overlay with a click-to-play flash video but clicking won't start the trailer.

This is going to be a relatively popular site, or at least high-profile, for the next 13 days. After the current offer is done the site may no longer demonstrate the problem.

	Michael Coates [:mcoates] (acct no longer active)
	Comment 62 • 12 years ago

Amazon video previews seem to be broken too. May be the same root cause.  If you attempt to watch the preview you'll see the "click here to activate the plugin" but after clicking nothing happens.


http://www.amazon.com/gp/product/B004E0FUBQ/ref=s9_hps_bw_g318_ir04?pf_rd_m=ATVPDKIKX0DER&pf_rd_s=center-7&pf_rd_r=0Z6CPD3460A0WART843C&pf_rd_t=101&pf_rd_p=1399870422&pf_rd_i=2676882011

Click on "play trailer"
Then click on "click here to activate the plugin"

	Loic Reporter
	Comment 63 • 12 years ago

(In reply to Michael Coates [:mcoates] from comment #62)
> Amazon video previews seem to be broken too.

It's bug 809903, I think.

	Benjamin Smedberg
	Comment 64 • 12 years ago

Please don't use this bug to list individual sites in which CTP is broken. This bug is about a specific pattern of instantiation. Test whether the tryserver builds for this bug fix the issue, and if not file a new bug.

	John Schoenick [:johns] Assignee
	Comment 65 • 12 years ago

Addressed review comments, and re-added addEventListener(PluginCrashed/PluginOutdated) that were overzealously removed. Carrying over r+

(In reply to Jared Wein [:jaws] from comment #50)
> > +function handleBindingAttached(evt) {
> > +  evt.target instanceof Ci.nsIObjectLoadingContent;
> > +  if (evt.target.pluginFallbackType == Ci.nsIObjectLoadingContent.PLUGIN_PLAY_PREVIEW)
>
> The first line here doesn't seem like it is doing what you wanted.

instanceof is actually magic with xpc objects, and is effectively a
QueryInterface call here. I moved it into the if statement to be less confusing
regardless.

	John Schoenick [:johns] Assignee
	Comment 66 • 12 years ago

So this depends on bug 548133 which depends on bug 810494 to ensure consistent bindings behavior.
Try of all of the above:
https://tbpl.mozilla.org/?tree=Try&rev=341e76ccebcc

The m-oth orange is due to accidentally dropping addEventListener(PluginCrashed) from patch 2 (it's not covered by these changes)

Try push to make sure that's fixed with updated patch 2:
https://tbpl.mozilla.org/?tree=Try&rev=ef8db14394ef

Assuming that is green these can all be landed and nominated

	John Schoenick [:johns] Assignee
	Comment 67 • 12 years ago

Argh we also want to keep the |plugin.clientTop| layout flush for just the
PluginCrashed event. I also opened bug 811520 for dealing with this bug in
regards to the pluginCrashed event, where it can still occur in odd edge cases

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 68 • 12 years ago

John reminded me I had some issues to address with this patch. I refactored the nested switch, so I'd appreciate another look at this from Margaret. Thanks!

	John Schoenick [:johns] Assignee
	Comment 69 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=7ac49c949717

Another try push with the fixed test failure and v2 of David's patch, for good measure.

	:Margaret Leibovic
	Comment 70 • 12 years ago

Comment on attachment 681294 [details] [diff] [review]
part 3 (v2) - fennec bits

Review of attachment 681294 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, I like this more. However, I have one concern about doorhangers for hidden plugins.

::: mobile/android/chrome/content/browser.js
@@ +5808,5 @@
> +          return;
> +        }
> +
> +        // If the plugin is hidden, or if the overlay is too small, show a 
> +        // doorhanger notification

Can you clarify what triggers the doorhanger now when there is no overlay? I didn't notice it on my previous review, but now we are bailing early if there is no overlay.

Also, nit: The first part of this comment isn't correct anymore.

	John Schoenick [:johns] Assignee
	Comment 71 • 12 years ago

(In reply to Margaret Leibovic [:margaret] from comment #70)
> Can you clarify what triggers the doorhanger now when there is no overlay? I
> didn't notice it on my previous review, but now we are bailing early if
> there is no overlay.

After we land bug 548133, the only case where we lack an overlay is display:none plugins, which will not trigger a doorhanger until they are shown -- however, display:none plugins cannot be instantiated regardless, so the doorhanger in this case would not successfully function.

	:Margaret Leibovic
	Comment 72 • 12 years ago

Comment on attachment 681294 [details] [diff] [review]
part 3 (v2) - fennec bits

Thanks for the clarification.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 73 • 12 years ago

Updated comment. Carrying over r+.

	John Schoenick [:johns] Assignee
	Comment 74 • 12 years ago

Comment on attachment 677912 [details] [diff] [review]
Part 1 - Remove binding events from nsObjectLoadingContent

https://hg.mozilla.org/integration/mozilla-inbound/rev/475674cbd9f1

	John Schoenick [:johns] Assignee
	Comment 75 • 12 years ago

Comment on attachment 681255 [details] [diff] [review]
Part 2 - Have the pluginProblem binding fire an event when bound, then use objlc.pluginFallbackType to determine what handler to use. r=jaws

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

	John Schoenick [:johns] Assignee
	Comment 76 • 12 years ago

Comment on attachment 681677 [details] [diff] [review]
part 3 (v2.1) - fennec bits

https://hg.mozilla.org/integration/mozilla-inbound/rev/0faf815f0b54

	John Schoenick [:johns] Assignee
	Comment 77 • 12 years ago

Comment on attachment 679492 [details] [diff] [review]
More test fixups

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

Try run for this whole series:
https://tbpl.mozilla.org/?tree=Try&rev=7ac49c949717

	Phil Ringnalda (:philor)
	Comment 78 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/475674cbd9f1
https://hg.mozilla.org/mozilla-central/rev/c8729d9d2d97
https://hg.mozilla.org/mozilla-central/rev/0faf815f0b54
https://hg.mozilla.org/mozilla-central/rev/cd8533b0fae7

	Loic Reporter
	Comment 79 • 12 years ago

(In reply to Daniel Veditz [:dveditz] from comment #61)
> Another broken site is https://www.humblebundle.com/ which is currently
> offering Android games. If you click on one of the game icons on the phone
> image you get an overlay with a click-to-play flash video but clicking won't
> start the trailer.

It works fine now, after this bug landed. So it's likely fixed, like my 2 examples in comment #0.

	Paul Silaghi, QA [:pauly]
	Comment 80 • 12 years ago

I confirm it's verified fixed on Nightly 20.0a1 (2012-11-20) Win 7 x64.

	John Schoenick [:johns] Assignee
	Comment 81 • 12 years ago

Comment on attachment 677912 [details] [diff] [review]
Part 1 - Remove binding events from nsObjectLoadingContent

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
N/A

User impact if declined: 
Click-to-play bustage on popular sites

Testing completed (on m-c, etc.): 
On m-c

Risk to taking this patch (and alternatives if risky): 
Moderate risk due to significant CTP changes, some event changes might conceivably break addons.

String or UUID changes made by this patch:
None

Note that this requires bug 548133 and bug 810494 to also be uplifted to land.

	John Schoenick [:johns] Assignee
	Comment 82 • 12 years ago

Comment on attachment 677912 [details] [diff] [review]
Part 1 - Remove binding events from nsObjectLoadingContent

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Known CTP bustage, which would affect our ability to enact CTP blocks on 17. If we want this for 17 we would also need to take bug 548133 and bug 810494

Fix Landed on Version:
20, likely being uplifted to 18+

String or UUID changes made by this patch:
None

User impact if declined: 
Risk to taking this patch (and alternatives if risky): 
See branches approval comment above

	Alex Keybl [:akeybl]
	Comment 83 • 12 years ago

Comment on attachment 677912 [details] [diff] [review]
Part 1 - Remove binding events from nsObjectLoadingContent

[Triage Comment]
Approving for landing in FF18, given our desire to use CTP with Flash in that timeframe. Let's get this landed sooner rather than later given the risk of regression here.

	Ryan VanderMeulen [:RyanVM]
	Comment 84 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/4238212c567c
https://hg.mozilla.org/releases/mozilla-aurora/rev/dbb0b90ea546
https://hg.mozilla.org/releases/mozilla-aurora/rev/8c83e26b3e0a
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf573f12023a

https://hg.mozilla.org/releases/mozilla-beta/rev/46430f63e85b
https://hg.mozilla.org/releases/mozilla-beta/rev/fb81a128125d
https://hg.mozilla.org/releases/mozilla-beta/rev/b0daef321ada
https://hg.mozilla.org/releases/mozilla-beta/rev/9b66328a0d4e

https://hg.mozilla.org/releases/mozilla-esr17/rev/a87449517d40
https://hg.mozilla.org/releases/mozilla-esr17/rev/46d794d296a4
https://hg.mozilla.org/releases/mozilla-esr17/rev/a66dabc85bce
https://hg.mozilla.org/releases/mozilla-esr17/rev/75febdeeef53

	Paul Silaghi, QA [:pauly]
	Comment 86 • 12 years ago

(In reply to Paul Silaghi [QA] from comment #80)
> I confirm it's verified fixed on Nightly 20.0a1 (2012-11-20) Win 7 x64.
Verified fixed on FF 18b3, Aurora 19.0a2 (2012-12-11), 2012-12-08-03-45-01-mozilla-esr17