Chat panels are lost while detaching/attaching

VERIFIED FIXED in Firefox 28

Status

defect
VERIFIED FIXED
6 years ago
5 months ago

People

(Reporter: cornel_ionce, Assigned: florian)

Tracking

Trunk
Firefox 29
Dependency tree / graph

Firefox Tracking Flags

(firefox28 verified, firefox29 verified)

Details

()

Attachments

(1 attachment, 3 obsolete attachments)

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.
(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.
(In reply to Florian Quèze [:florian] [:flo] from comment #1)

> Step 5 isn't needed here.

Scratch this, I was confused.
Posted patch Patch (obsolete) — Splinter Review
Assignee: nobody → florian
Status: NEW → ASSIGNED
Attachment #8356112 - Flags: review?(mixedpuppy)
Posted patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #8356112 - Attachment is obsolete: true
Attachment #8356112 - Flags: review?(mixedpuppy)
Attachment #8356146 - Flags: review?(mixedpuppy)
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 on attachment 8356146 [details] [diff] [review]
Patch v2

lgtm, try run?
Attachment #8356146 - Flags: review?(mixedpuppy) → review+
Duplicate of this bug: 923242
Posted patch Patch v3 (obsolete) — Splinter Review
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.)
Attachment #8356146 - Attachment is obsolete: true
Attachment #8357129 - Flags: review?(mixedpuppy)
Posted patch Patch V4Splinter Review
helped fix async test failure

https://tbpl.mozilla.org/?tree=Try&rev=e0c52ea997bf
Attachment #8357129 - Attachment is obsolete: true
Attachment #8357129 - Flags: review?(mixedpuppy)
Attachment #8359734 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/76d183dbdaad
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Cornel, please verify this is working in tomorrow's nightly.
Flags: needinfo?(cornel.ionce)
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.
Attachment #8359734 - Flags: approval-mozilla-aurora?
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).
Status: RESOLVED → VERIFIED
Flags: needinfo?(cornel.ionce)
Attachment #8359734 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Cornel, please verify in the latest Aurora.
Flags: needinfo?(cornel.ionce)
Keywords: verifyme
QA Contact: cornel.ionce
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.
Flags: needinfo?(cornel.ionce)
Keywords: verifyme
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.