853615 - Dragging tab out of window breaks CTP

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Lucas Adamski [:ladamski]]

I have click to play enabled for all plugins.

Visit http://www.youtube.com/watch?v=4wpHan_g_R4&feature=youtu.be in a new tab in an existing window (which contains other tabs).

Tear the tab out into a new window.  It can stay on the same monitor.  Now click to play the video.  The layer receives the click but the video doesn't play and the CTP overlay remains.

If you click to play without first tearing the tab out of the existing window, it plays normally.

Comment 1

[Paul Silaghi, QA [:pauly]]

Last good nightly: 2012-11-27
First bad nightly: 2012-11-28

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=597915b66059&tochange=3c3a8eed0578

Comment 2

[Georg Fritzsche [:gfritzsche]]

Paul, is ESR17 affected?

Comment 3

[Paul Silaghi, QA [:pauly]]

(In reply to Georg Fritzsche [:gfritzsche] [at Performance work-week] from comment #2)
> Paul, is ESR17 affected?
No. Tested with 2013-03-20-03-45-01-mozilla-esr17.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

It may be too late for FF20 unless this regression can be fixed with a low risk backout - David, would bug 746374 be a place to begin investigation on how to resolve this?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

This is what I'm seeing:

WARNING: Not same origin error!: file /home/keeler/mozilla-central/dom/base/nsJSEnvironment.cpp, line 386
JavaScript error: chrome://browser/content/browser.js, line 3232: browser is null

3222: canActivatePlugin: function PH_canActivatePlugin(objLoadingContent) {
...
3231: let browser = gBrowser.getBrowserForDocument(objLoadingContent.ownerDocument.defaultView.top.document);
3232: let pluginPermission = Services.perms.testPermission(browser.currentURI, permissionString);

Replacing those two lines with the following fixes the immediate issue:

  let currentURI = makeURI(objLoadingContent.ownerDocument.defaultView.top.document.documentURI);
  let pluginPermission = Services.perms.testPermission(currentURI, permissionString);

But a) I'm concerned that other uses of gBrowser.getBrowserForDocument(...) will fail in similar situations and b) this exposes another bug when activating plugins of one type using the popup notification. This line fails:

(browser-plugins.js:629) aBrowser._clickToPlayPluginsActivated.set(this.message, true);

with _clickToPlayPluginsActivated being undefined. This should be set in browser.js:4696 and would hopefully carry over when dragging/dropping a tab, but apparently it doesn't? This is unfortunate, because it means we lose all state with respect to which plugin types have been activated on that page.

Cc'ing Jared and Tim - maybe they can shed some light on what's going on.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

We'll have to wontfix this for FF20 since we're building our final beta (and subsequent RC) today

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

The problem here is that when clicking the overlay, the event is handled by the old browser window. That is why gBrowser.getBrowserForDocument() returns null - the document is in the other window. When tearing off the tab we need to unregister the click event handler and register a new one for the new window.

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

Thanks for the info.
Here's a patch. I'm not entirely happy with how it works, but either it's the best of a not great situation or someone will have ideas as to how to improve it.
Tim, would you be able to review this?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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

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

Sorry for the late response, redirecting the request to someone with more CTP XP :)

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

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

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

Hm, it seems that there are more issues at play than just this one that is being fixed. Can any gBrowser references within browser-plugins.js be trusted? For this bug I'm most concerned with http://hg.mozilla.org/mozilla-central/annotate/b0d842380959/browser/base/content/browser-plugins.js#l235. I'm clearing the review request based on this, please reflag for review after you have researched this a bit more.

This also means that the plugin crash UI is probably broken in the same way, although it is far more of an edge-case since the user would have to tear off the tab after the plugin has crashed. We should get bugs on file for the other parts that might be broken.

::: browser/base/content/browser-plugins.js
@@ +479,5 @@
> +  _overlayClickListener: {
> +    handleEvent: function PH_handleOverlayClick(aEvent) {
> +      let plugin = document.getBindingParent(aEvent.target);
> +      let contentWindow = plugin.ownerDocument.defaultView.top;
> +      let browser = gBrowser.getBrowserForDocument ?

In what cases does getBrowserForDocument not exist?

@@ +570,5 @@
> +    let plugins = cwu.plugins;
> +    for (let plugin of plugins) {
> +      let overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> +      if (overlay)
> +        overlay.removeEventListener("click", gPluginHandler._overlayClickListener, true);

Can you explain why the click event handler should be removed in this case? reshowClickToPlayNotification is called when navigating in the BF cache, so the browser window should stay the same. 

What should happen after this call? Will clicks on the plugin still activate it?

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2

(In reply to Jared Wein [:jaws] from comment #10)
> Comment on attachment 732946 [details] [diff] [review]
> patch
> 
> Review of attachment 732946 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hm, it seems that there are more issues at play than just this one that is
> being fixed. Can any gBrowser references within browser-plugins.js be
> trusted? For this bug I'm most concerned with
> http://hg.mozilla.org/mozilla-central/annotate/b0d842380959/browser/base/
> content/browser-plugins.js#l235. I'm clearing the review request based on
> this, please reflag for review after you have researched this a bit more.

Since that's not in an event listener callback, it should be safe. I extended the test to specifically check that it's okay (and from my local testing, it is).

> This also means that the plugin crash UI is probably broken in the same way,
> although it is far more of an edge-case since the user would have to tear
> off the tab after the plugin has crashed. We should get bugs on file for the
> other parts that might be broken.

Filed bug 863827.

> ::: browser/base/content/browser-plugins.js
> @@ +479,5 @@
> > +  _overlayClickListener: {
> > +    handleEvent: function PH_handleOverlayClick(aEvent) {
> > +      let plugin = document.getBindingParent(aEvent.target);
> > +      let contentWindow = plugin.ownerDocument.defaultView.top;
> > +      let browser = gBrowser.getBrowserForDocument ?
> 
> In what cases does getBrowserForDocument not exist?

I added a comment in the patch, but to reiterate here: If a tab is drag-and-dropped from a window containing only that tab, that window goes away, and hence so does gBrowser.getBrowserForDocument.

> @@ +570,5 @@
> > +    let plugins = cwu.plugins;
> > +    for (let plugin of plugins) {
> > +      let overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> > +      if (overlay)
> > +        overlay.removeEventListener("click", gPluginHandler._overlayClickListener, true);
> 
> Can you explain why the click event handler should be removed in this case?
> reshowClickToPlayNotification is called when navigating in the BF cache, so
> the browser window should stay the same. 
> 
> What should happen after this call? Will clicks on the plugin still activate
> it?

Well, this is an attempt to remove the old, not-going-to-work click handler. The problem is I don't know a way to differentiate a tab drag-and-drop page show event (and hence a reshowClickToPlayNotification call) from a loaded-from-BF-cache event. Ideas?

What should happen here is it removes the old, invalid click handler and registers a new, good one. Clicks on the plugin work as expected, then (i.e. activating the plugin or showing the popup notification).

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

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

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

Cool, thanks.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thanks for the quick review!

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

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8130599b8886

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8130599b8886

Comment 16

[bhavana bajaj [:bajaj]]

Hey David,Can you please request approval for aurora asap on this ?

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 746374
User impact if declined: users will have to refresh tabs they've drag-and-dropped in order for click-to-play to work on those tabs
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): it only touches click-to-play code in browser.js. In theory it could break ctp, but I've seen no evidence of that (no new bugs or regressions so far).
String or IDL/UUID changes made by this patch: none

Comment 18

[Alex Keybl [:akeybl]]

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

Approving for Aurora but not Beta - we'll maintain the status quo since we don't like to touch plugin code in our final couple of betas if at all possible :)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/22e3598b08d8

Comment 20

[Paul Silaghi, QA [:pauly]]

Verified fixed 22.0a2 (2013-04-29), 23.0a1 (2013-04-29) Win 7, Ubuntu 12.04, Mac OS X 10.8.3