1250534 - Use a surefire, e10s-compatible method to receive a signal when a chat window is closed in attached and detached mode

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mike de Boer [:mikedeboer]]

We have the following:

1) a browser window that opens a .xul document with `window.openDialog()`
2) inside that XUL document is a <browser> element that's part of a messagemanagergroup.
3) there's a frame script loaded that listens to the 'DOMWindowClose' event.

When you close that dialog window, the DOMWindowClose event does not fire. The expected behavior is that is should.

This bug is currently affecting Firefox Hello in such a way that our conversation cleanup code doesn't run when a detached conversation (in a dialog window) is closed.

Comment 1

[:Gijs (he/him)]

Wouldn't the domwindowclose event fire on the XUL document, not on the content? So unless someone in the parent forwards it to the child process it won't know it happened.

Comment 2

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #1)
> Wouldn't the domwindowclose event fire on the XUL document, not on the
> content? So unless someone in the parent forwards it to the child process it
> won't know it happened.

I'm pretty sure it'll be 'capturable' on the XUL document, but it _used_ to work on <browser> elements contained in that DOMWindow too. I'd say this was supported, because <browser> elements are usually closed using controls from chrome code (tab close button, hotkeys, etc).
At least it's the case that Social API is relying on this functionality to work like described. When talking to smaug on IRC, it seemed like he agreed this being a regression of existing functionality. It is certainly not something covered by regression tests, but that can be fixed along with this bug.

Comment 3

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Wouldn't the domwindowclose event fire on the XUL document, not on the
> > content? So unless someone in the parent forwards it to the child process it
> > won't know it happened.
> 
> I'm pretty sure it'll be 'capturable' on the XUL document, but it _used_ to
> work on <browser> elements contained in that DOMWindow too. I'd say this was
> supported, because <browser> elements are usually closed using controls from
> chrome code (tab close button, hotkeys, etc).
> At least it's the case that Social API is relying on this functionality to
> work like described. When talking to smaug on IRC, it seemed like he agreed
> this being a regression of existing functionality. It is certainly not
> something covered by regression tests, but that can be fixed along with this
> bug.

You're aware social API has its own wrappers (in MozSocialAPI.jsm) and also checks for a message sent from the message manager, right? Is it clear which portion of this is currently not working? As in, do you know if the event fires on the XUL document right now? What about the message? Is browser-child.js loaded in the frame in the <browser> ?

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs Kruitbosch from comment #3)
> You're aware social API has its own wrappers (in MozSocialAPI.jsm) and also
> checks for a message sent from the message manager, right?

Those are the ones I'm attempting to use at the moment. It works for chatboxes attached to the chatbar - 'DOMWindowClose' is fired when the binding instance is removed from its parent <chatbar> element - but does not work in the detached case - not fired when the popup dialog window is closed.

> Is it clear which
> portion of this is currently not working? As in, do you know if the event
> fires on the XUL document right now? What about the message? Is
> browser-child.js loaded in the frame in the <browser> ?

Since everything is working as expected when a chatbox is attached to the chatbar, I think it's quite clear what's not working: there's (most) likely a timing issue when a window is closed; it happens too fast for the DOMWindowClose event to reach the event handler in the frame script.
All scripts that should load with the message group as specified have been loaded.

Comment 5

[Shane Caraveo (:mixedpuppy)]

I'm dredging up some 2-3 year old memories here, things could have changed, or my memory may not be quite right (ie. lots of caveats here).  I seem to recall that window.close is somehow synchronous and using async messaging didn't work well during this event (as this bug seems to replicate).

Comment 6

[Mark Banner (:standard8)]

This means that Loop doesn't cleanup conversations correctly if they have been popped-out of the main window, when in e10s mode. Hence we really need this fixed.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

After thinking this some more, relying on DOMWindowClose in a frame script doesn't quite make sense. It is something which should happen in parent process, since that is being closed. And we don't do sync parent->child messaging so that even if child got the event, it was happening rather late.
One way to deal with this is to prevent closing in parent, send message to child that window is being closed and then really close the window in parent.

nsGlobalWindow::ForceClose() dispatches DOMWindowClose only in the parent process and
nsGlobalWindow::CloseOuter only when window.close() is explicitly called in JS, and in that case frame script does actually get DOMWindowClose as expected.
So I would say things are working as expected from core code side.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

But it is possible that I don't quite understand the use case for which DOMWindowClose is used here.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Since I thought originally this being about the case where user clicks top-right [x], but DOMWindowClose in social code isn't used for that but for window.close() as expected.
So what is this bug about actually?

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Olli Pettay [:smaug] from comment #7)
> After thinking this some more, relying on DOMWindowClose in a frame script
> doesn't quite make sense. It is something which should happen in parent
> process, since that is being closed. And we don't do sync parent->child
> messaging so that even if child got the event, it was happening rather late.
> One way to deal with this is to prevent closing in parent, send message to
> child that window is being closed and then really close the window in parent.
> 
> nsGlobalWindow::ForceClose() dispatches DOMWindowClose only in the parent
> process and
> nsGlobalWindow::CloseOuter only when window.close() is explicitly called in
> JS, and in that case frame script does actually get DOMWindowClose as
> expected.
> So I would say things are working as expected from core code side.

I think you're understanding my question, don't worry :-)
The gist of this is that we were relying on the DOMWindowClose event to bubble down to browser element children of a XUL window, but this is not something we should do.
Instead I will work on a solution where we don't need to rely on this behavior.

Thanks for the explanation, Olli!

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: introduce a ChatboxClosed event that fires when a chatbox is closed

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8728103 [details] [diff] [review]
Patch v1: introduce a ChatboxClosed event that fires when a chatbox is closed

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

::: browser/extensions/loop/chrome/content/modules/MozLoopService.jsm
@@ +1057,5 @@
> +            listeners = {};
> +
> +            windowCloseCallback();
> +
> +            listeners = null;

I removed this line locally.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8728103 [details] [diff] [review]
Patch v1: introduce a ChatboxClosed event that fires when a chatbox is closed

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

Ok, looks good. r=Standard8.

Please land in m-c and I'll import into our repo around the time it hits central.

Comment 14

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/5b2dc63c466a8a37141096d5e1c61250d47fac0f
Bug 1250534: introduce a ChatboxClosed event that fires when a chatbox is closed in attached and detached mode. r=Standard8

Comment 15

[GitHub Autolander Bot]

Attachment: [loop] mikedeboer:bug-1250534-ChatboxClosedEvent > mozilla:master

Comment 16

[Mark Banner (:standard8)]

https://github.com/mozilla/loop/commit/2bc0989a1fa9d3004627b27900f29e01c70fce5f
https://github.com/mozilla/loop/commit/99f6826d88f470f42efc714d4079000202621e85

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b2dc63c466a

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8728103 [details] [diff] [review]
Patch v1: introduce a ChatboxClosed event that fires when a chatbox is closed

Approval Request Comment
[Feature/regressing bug #]: Firefox Hello e10s changes
[User impact if declined]: Popping-out the conversation window, and then trying to close the conversation doesn't work properly - the conversation is closed, but the main Firefox UI is left in a bad state that confuses the used.
[Describe test coverage new/current, TreeHerder]: Landed in nightly builds, tested and verified locally.
[Risks and why]: Low, minor change affecting only the conversation window that Hello is the only user of.
[String/UUID change made/needed]: None

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8728103 [details] [diff] [review]
Patch v1: introduce a ChatboxClosed event that fires when a chatbox is closed

Fix for e10s related regression, let's take this for aurora and beta

Comment 20

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/831b9b1f8df2

Comment 21

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/mozilla-beta/rev/d574b8fcd20f

Comment 22

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(In reply to Mark Banner (:standard8) from comment #18)
> Comment on attachment 8728103 [details] [diff] [review]
> Patch v1: introduce a ChatboxClosed event that fires when a chatbox is closed
> 
> Approval Request Comment
> [Feature/regressing bug #]: Firefox Hello e10s changes
> [User impact if declined]: Popping-out the conversation window, and then
> trying to close the conversation doesn't work properly - the conversation is
> closed, but the main Firefox UI is left in a bad state that confuses the
> used.
Hello! I want to understand how does the "main Firefox UI bad state" manifest, in order to reproduce and verify this issue. Thank you!

Comment 23

[Mark Banner (:standard8)]

(In reply to Iulia Cristescu, QA [:IuliaC] from comment #22)
> (In reply to Mark Banner (:standard8) from comment #18)
> > Comment on attachment 8728103 [details] [diff] [review]
> > Patch v1: introduce a ChatboxClosed event that fires when a chatbox is closed
> > 
> > Approval Request Comment
> > [Feature/regressing bug #]: Firefox Hello e10s changes
> > [User impact if declined]: Popping-out the conversation window, and then
> > trying to close the conversation doesn't work properly - the conversation is
> > closed, but the main Firefox UI is left in a bad state that confuses the
> > used.
> Hello! I want to understand how does the "main Firefox UI bad state"
> manifest, in order to reproduce and verify this issue. Thank you!

iirc it would either not close the pop-out window, or it would close it. In either case, the sharing toolbar was left open, and the Hello panel would say that you were in a room. However, you could not disconnect from either of those.

Comment 24

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I verified this issue on Windows 10 x6, Windows 7 X64, Ubuntu 14.04 x86  and on Mac OS X 10.10.5. These  are the results:
- 46.0b10 build1 (20160411042519)-- for e10s disabled, the issue is fixed
                                 -- for e10s and loop.remote.autostart enabled, clicking the detach button stops the conversation from the host side - Bug 1257536
- 47.0a2 (2016-04-13) -- since the detach button was disabled, this fix does not apply to it
- 48.0a1 (2016-04-13) -- e10s is enabled by default, clicking the detach button stops the conversation from the host side and causes the standalone side to crash - Bug 1263667 
Will retest the issue after the above mentioned bugs will be fixed.

Comment 25

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I tried again to verify this issue (after the Bug 1263667 was fixed) on 
- 48.0a1 (2016-04-29) 
- 49.0a1 (2016-04-28) 
using 
- Windows 10 x86 
- Ubuntu 14.04 x86 
- Mac OS X 10.9.5 
These are the results:
- clicking the detach button stops the conversation from the host side (this is the state received from the the remote side perspective)
- the host conversation window clears its content
- the host side can't disconnect, only if closing the conversation window in detached mode

Comment 26

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I verified again this bug on 
- 46.0.1 build1 (20160502172042)
- 47.0b8 build1 (20160523113146)
- latest Aurora 48.0a2 (2016-05-26)
- latest Nightly 49.0a1 (2016-05-25)
using
- Ubuntu 12.04 x86
- Windows 10 x64
I used the pref loop.server set on https://loop.dev.mozaws.net/v0. The results are as follows:
- 46.0.1 build1  - the issue is fixed (e10s off / e10s on)
- 47.0b8 build1  - since the detach button was disabled, this fix does not apply to it
- latest Aurora  - clicking the detach button stops the conversation from the host side (this is the state received from the the remote side perspective)
                 - the host conversation window clears its content
                 - the host side can't disconnect, only if closing the conversation window in detached mode
- latest Nightly - since the detach button was disabled, this fix does not apply to it
In conclusion, Firefox 48 is still affected.

Comment 27

[Mark Banner (:standard8)]

(In reply to Iulia Cristescu, QA [:IuliaC] from comment #26)
> - latest Nightly - since the detach button was disabled, this fix does not
> apply to it
> In conclusion, Firefox 48 is still affected.

Disabling the button in 48 is going to be done once bug 1273671 gets approval for uplift.

Comment 28

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I tested this issue after the land of the Loop 1.4.0 system add-on (bug 1273671) in Firefox 48.0b1 build2 (20160606200529) using 
- Windows 10 x64
- Ubuntu 14.04 x86
- Mac OS X 10.11.
The overall feature state seems to be the expected one and I encountered no misbehavior between the last and the present implementation. Also, the detach button is disabled and so, the problem in question is resolved.
I will update the Tracking Flags, according to this and the previous comments.