Closed Bug 853615 Opened 7 years ago Closed 7 years ago

Dragging tab out of window breaks CTP

Categories

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

21 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox20 + wontfix
firefox21 + wontfix
firefox22 --- verified
firefox23 --- verified
firefox-esr17 --- unaffected

People

(Reporter: ladamski, Assigned: keeler)

References

Details

(Keywords: regression, Whiteboard: [CtPDefault:P1])

Attachments

(1 file, 1 obsolete file)

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.
No longer depends on: 840944
Whiteboard: [CtPDefault:P2]
Blocks: 841350
Last good nightly: 2012-11-27
First bad nightly: 2012-11-28

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=597915b66059&tochange=3c3a8eed0578
Paul, is ESR17 affected?
Flags: needinfo?(paul.silaghi)
Priority: P3 → P2
Whiteboard: [CtPDefault:P1]
(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.
Flags: needinfo?(paul.silaghi)
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?
Assignee: nobody → dkeeler
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.
We'll have to wontfix this for FF20 since we're building our final beta (and subsequent RC) today
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.
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #732946 - Flags: review?(ttaubert)
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 :)
Attachment #732946 - Flags: review?(ttaubert) → review?(jAwS)
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?
Attachment #732946 - Flags: review?(jAwS)
Attached patch patch v2Splinter Review
(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).
Attachment #732946 - Attachment is obsolete: true
Attachment #739752 - Flags: review?(jaws)
Comment on attachment 739752 [details] [diff] [review]
patch v2

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

Cool, thanks.
Attachment #739752 - Flags: review?(jaws) → review+
https://hg.mozilla.org/mozilla-central/rev/8130599b8886
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Hey David,Can you please request approval for aurora asap on this ?
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
Attachment #739752 - Flags: approval-mozilla-beta?
Attachment #739752 - Flags: approval-mozilla-aurora?
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 :)
Attachment #739752 - Flags: approval-mozilla-beta?
Attachment #739752 - Flags: approval-mozilla-beta-
Attachment #739752 - Flags: approval-mozilla-aurora?
Attachment #739752 - Flags: approval-mozilla-aurora+
Verified fixed 22.0a2 (2013-04-29), 23.0a1 (2013-04-29) Win 7, Ubuntu 12.04, Mac OS X 10.8.3
Status: RESOLVED → VERIFIED
OS: Windows 7 → All
Duplicate of this bug: 886380
You need to log in before you can comment on or make changes to this bug.