841350 - Drag and drop a CTP tab in a new window makes the CTP icon disappear

Status: VERIFIED FIXED

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

Description

[Paul Silaghi, QA [:pauly]]

1. Enable CTP
2. Open 2 youtube videos in 2 tabs
3. Detach a tab to create a new FF window (or drag and drop the tab into an existing window)

AR: The CTP icon is gone after the drag & drop.

Reproducible on FF 19b6, 20.0a2 (2013-02-13), 21.0a1 (2013-02-13).
Not a regression, reproducible on FF 16.0.2, 17.0.1.

Comment 1

[Paul Silaghi, QA [:pauly]]

May be related to bug 820448

Comment 2

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

Attachment: possible fix

The code that moves a tab from one browser to another doesn't take popup notifications into account. One option would be to re-architect popup notifications to be more tab-moving friendly. Another option would be to reshow any notifications we care about upon a tab being moved, as I've done here.

Comment 3

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

Comment on attachment 714111 [details] [diff] [review]
possible fix

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

Tim, what do you think about the TabSwappedBrowsers event?

::: browser/base/content/browser-plugins.js
@@ +505,5 @@
>      }, true);
>    },
>  
> +  reshowClickToPlayNotification: function PH_reshowClickToPlayNotification(aBrowser) {
> +    let browser = aBrowser ? aBrowser : gBrowser.selectedBrowser;

let browser = aBrowser || gBrowser.selectedBrowser;

@@ +513,5 @@
>  
>    // returns true if there is a plugin on this page that needs activation
>    // and isn't in the "except these" list
> +  _pluginNeedsActivationExceptThese: function PH_pluginNeedsActivationExceptThese(aExceptThese, aBrowser) {
> +    let browser = aBrowser ? aBrowser : gBrowser.selectedBrowser;

let browser = aBrowser || gBrowser.selectedBrowser;

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 714111 [details] [diff] [review]
possible fix

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

Moving a tab to a different window by using gBrowser.swapBrowsersAndCloseOther() fires pageshow events so we should actually have the call to reshowClickToPlayNotification() for free.

The right fix would be to register the 'pageshow' handler:

> gBrowser.addEventListener("pageshow", function(event) {
>   // Filter out events that are not about the document load we are interested in
>   if (content && event.target == content.document)
>     setTimeout(pageShowEventHandlers, 0, event);
> }, true);

before we call swapBrowsersAndCloseOther() in the browser's delayed startup. So we only need to move the code above a couple of lines up.

Comment 5

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

Attachment: patch

Well, I got this to work, but I had to relax the restriction that event.target == content.document. For some reason, when moving a tab from its own window to an existing window, event.target would never equal content.document. I tried to figure out why, but to no avail. Tim - is this okay?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

So, for some reason, (event.target == content.document) is true if you check it on the next tick like here:

> gBrowser.addEventListener("pageshow", function(event) {
>   setTimeout(function () {
>     if (content && event.target == content.document)
>       setTimeout(pageShowEventHandlers, 0, event);
>   }, 0);
> }, true);

That's not a solution of course and I assume this is caused by docShell swapping magic and may be a platform bug.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

In particular, content.document seems to change.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

The problem is that the XULWindow::mPrimaryContentShell isn't up-to-date when the pageshow is fired. This field is updated here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#872

... but updateCurrentBrowser() is called *after* browser.swapDocShells() - which is what fires pageshow events. So... an easy solution would be to move the (content.document == event.target) check to the beginning of pageShowEventHandlers(). After setTimeout() (i.e. on the next tick) everything is as it should be.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

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

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

Please take a look at comment #8. If you also add a nice comment why that check is there and that setTimeout() is important we should be fine.

Comment 10

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

Attachment: patch v2

Thanks for your help on this - I think it's ready for an actual review.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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

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

Thanks! r=me with the comment fixed.

::: browser/base/content/browser.js
@@ +1342,5 @@
>  
>      var isLoadingBlank = isBlankPageURL(uriToLoad);
>  
> +    gBrowser.addEventListener("pageshow", function(event) {
> +      // The XULWindow::mPrimaryContentShell isn't up-to-date when pageshow

Please mention that this is only relevant when the pageshow event was fired by a swapFrameLoaders() call, i.e. when we swap the docShells of two tabs.

Comment 12

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

Attachment: patch v2 (fixed comment)

Thanks! Carrying over r+.
https://tbpl.mozilla.org/?tree=Try&rev=43c4a343d0f2

Comment 13

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

Attachment: patch v2.1

Had to fix the test, but nothing else changed, so carrying over r+.
https://tbpl.mozilla.org/?tree=Try&rev=789f706e3ee6
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fb9e70a38b2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4fb9e70a38b2

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> So... an easy solution would be to move
> the (content.document == event.target) check to the beginning of
> pageShowEventHandlers(). After setTimeout() (i.e. on the next tick)
> everything is as it should be.

This workaround likely caused bug 847070. Seems like we should fix the underlying event ordering issue rather than trying to work around it.

Comment 16

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

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/aceda400d212

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aceda400d212

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Ok, let's fix this for real. I investigated this a little more and think I found the actual issue here.

1) When dragging a tab off the strip into a new window, the <tab> element does not have the type="content-primary" attribute that selected tabs should have. That's easy to add for the first default tab in browser.xul.

2) When dragging a tab to another (already existing) window a new tab is created and the docShells are swapped. However, the newly created tab is not selected when the swapping starts so we must ensure that it is and just call updateCurrentBrowser() afterwards again.

The actual fix of registering the pageshow listener earlier is of course still needed.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

Sorry for stealing this, David. I thought it might be easier to just submit the patch I wrote while looking for a new solution instead of explicitly writing down what needs to be done :) It'd be great if you can take a look at this and confirm that this fixes your problem!

Comment 20

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

Comment on attachment 721302 [details] [diff] [review]
patch v3

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

Works for me! Thanks for taking this on. It would have taken me a while to figure out how to fix this properly. I'll upload a patch with just the test from the previous patch.

Comment 21

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

Attachment: test from patch v2.1

This is just the test that was in patch v2.1. Carrying over r+.

Comment 22

[Frank Yan (:fryn)]

Comment on attachment 721302 [details] [diff] [review]
patch v3

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

::: browser/base/content/browser.xul
@@ +909,5 @@
>              flex="1"
>              setfocus="false"
>              tooltip="tabbrowser-tab-tooltip"
>              stopwatchid="FX_TAB_CLICK_MS">
> +        <tab class="tabbrowser-tab" selected="true" fadein="true" type="content-primary"/>

(In reply to Tim Taubert [:ttaubert] from comment #18)
> 1) When dragging a tab off the strip into a new window, the <tab> element
> does not have the type="content-primary" attribute that selected tabs should
> have. That's easy to add for the first default tab in browser.xul.

As I understand it, we don't set type="content-primary" on any <tab>. We only set it on <browser> elements. This change looks bogus to me.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 721302 [details] [diff] [review]
patch v3

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+    gBrowser.addEventListener("pageshow", function(event) {

Worth adding a comment that this needs to be done before the swapBrowsersAndCloseOther call below.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Frank Yan (:fryn) from comment #22)
> As I understand it, we don't set type="content-primary" on any <tab>. We
> only set it on <browser> elements. This change looks bogus to me.

Thanks for catching this, it looks indeed bogus and unnecessary. The patch still works without it, not sure why it ended up in this patch.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

This seems to reliably break browser_CTPScriptPlugin.js:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTPScriptPlugin.js | waited too long for popup notification to show (plugin_test_scriptedPopup3.html)

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

Comment 26

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

Attachment: fix browser_CTPScriptPlugin.js

It turns out the test wasn't quite doing the right thing.
I've used this pattern elsewhere, so it might be worth going back and fixing those too...

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 725029 [details] [diff] [review]
fix browser_CTPScriptPlugin.js

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

Ah, thanks for the quick fix! It probably makes sense to fix the other tests, too (in a follow-up). I'll push it all to try again.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Looking good.

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

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/7cc974e3a6a3

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7cc974e3a6a3

Comment 31

[tabmix.onemen]

Error: TypeError: this.updateCurrentBrowser is not a function
Source file: chrome://browser/content/tabbrowser.xml
Line: 3855

-   this.updateCurrentBrowser(true);
+   this.tabbrowser.updateCurrentBrowser(true);

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

Oops :/ Thanks for noticing. I'll fix it right away.

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/a53f03e0eeda

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a53f03e0eeda

Comment 35

[:Gavin Sharp [email: [e-mail]]]

Hrm, no tests failing for that one? We should fix that!

Comment 36

[Paul Silaghi, QA [:pauly]]

Verified fixed in nightly 22.0a1 (2013-03-24)