911981 - Chat panels are lost while detaching/attaching

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Cornel Ionce [:noni] [Hubs QA]]

STR:
1. Open Firefox
2. Go to http://mixedpuppy.github.io/socialapi-demo/ and select "Activate The Demo Provider"
3. Click on Chat Panel twice.
4. Play the video on the first chat (to be able to distinguish chats after the next step)
5. Detach the first opened chat and reattach it.
6. Detach the second chat and reattach it.

Expected Result:
Both chats are reattached and shown side by side.

Actual Result:
The first chat (with the video playing) is lost, only the second one is displayed.


Build ID: 20130902131354 (Firefox 24 beta 8)

User Agents:
Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Firefox/24.0

Notes:
This reproduces if more two or more chat panels are opened.
This issue is not a regression.
This issue is also reproducing on latest Nightly and Aurora.

Comment 1

[Florian Quèze [:florian]]

(In reply to Cornel Ionce [QA] from comment #0)
> STR:

> 5. Detach the first opened chat and reattach it.
> 6. Detach the second chat and reattach it.

Step 5 isn't needed here.

Comment 2

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)

> Step 5 isn't needed here.

Scratch this, I was confused.

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch

Comment 4

[Florian Quèze [:florian]]

Attachment: Patch v2

After more testing, I've found a second issue related to attaching/detaching multiple times, STRs:
1. Open 2 chat windows.
2. Detach one.
3. Detach the other.

Expected result:
2 detached chat windows

Actual result:
The content of the first detached chat window has been replaced by a blank area. The second chat window is still attached.


The fix for this is simple: by default openDialog won't create more than one window with the same name (null here). Using the special _blank value fixes it and ensures we always get a new window.

I've adapted the test to cover both this issue and the issue in comment 0.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8356146 [details] [diff] [review]
Patch v2

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

::: browser/base/content/test/social/browser_chat_tearoff.js
@@ +268,5 @@
> +                waitForCondition(() => chats.children.length == 0, function () {
> +                  info("no chat window left");
> +                  next();
> +                });
> +                return;

This 'return;' statement is pointless. I'll remove it for the next version, or before doing the check-in if there's nothing else to address.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8356146 [details] [diff] [review]
Patch v2

lgtm, try run?

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch v3

I pushed attachment 8356146 [details] [diff] [review] to try (https://tbpl.mozilla.org/?tree=Try&rev=ebe57ddbf83d) and there were consistent "browser_social_chatwindow.js | chatboxForURL map should be empty - Got 1, expected 0" failures.

This updated patch does a better job at cleaning up the chatboxForURL map.
New patch pushed to try at https://tbpl.mozilla.org/?tree=Try&rev=e8bd1618803e (I don't have the results yet, but the tests passed locally.)

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: Patch V4

helped fix async test failure

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

Comment 10

[Florian Quèze [:florian]]

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

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/76d183dbdaad

Comment 12

[u279076]

Cornel, please verify this is working in tomorrow's nightly.

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8359734 [details] [diff] [review]
Patch V4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 880911
User impact if declined: some chat windows lost in attach/detach operations.
Testing completed (on m-c, etc.): on m-c, with automated tests.
Risk to taking this patch (and alternatives if risky): low.
String or IDL/UUID changes made by this patch: none.

Comment 14

[Cornel Ionce [:noni] [Hubs QA]]

User Agents:
Mozilla/5.0 (Windows NT 6.1; rv:29.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (Windows NT 6.3; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:29.0) Gecko/20100101 Firefox/24.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/24.0

Verified as fixed on latest Nightly (Build ID:20140116030250).

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c39ef9346ede

Comment 16

[u279076]

Cornel, please verify in the latest Aurora.

Comment 17

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (X11; Linux i686; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.1; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.3; rv:28.0) Gecko/20100101 Firefox/28.0

Verified as fixed on latest Aurora (build ID: 20140121004017) using the STR from the description.