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

VERIFIED FIXED in Firefox 46

Status

defect
P1
major
Rank:
14
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

unspecified
mozilla48
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(e10sm9+, firefox45 wontfix, firefox46 verified, firefox47 verified, firefox48 verified)

Details

(Whiteboard: [e10s][btpp-active])

Attachments

(2 attachments)

Assignee

Description

3 years ago
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.
Flags: qe-verify+
Flags: firefox-backlog+

Comment 1

3 years ago
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.
Assignee

Comment 2

3 years ago
(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

3 years ago
(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> ?
Assignee

Comment 4

3 years ago
(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.
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).
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.
tracking-e10s: --- → ?
Flags: needinfo?(mconley)
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.
But it is possible that I don't quite understand the use case for which DOMWindowClose is used here.
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?
Assignee

Comment 10

3 years ago
(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!
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 48.1 - Mar 21
Points: --- → 3
Assignee

Comment 12

3 years ago
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.
Assignee

Updated

3 years ago
Component: General → Client
Flags: needinfo?(mconley)
Product: Firefox → Hello (Loop)
Version: Trunk → unspecified
Assignee

Updated

3 years ago
Summary: DOMWindowClose event captured in a frame script of a browser element inside a dialog window doesn't fire → Use a surefire, e10s-compatible method to receive a signal when a chat window is closed in attached and detached mode
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.
Attachment #8728103 - Flags: review?(standard8) → review+
Rank: 14
Whiteboard: [e10s] → [e10s][btpp-active]
Assignee

Comment 14

3 years ago
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
Depends on: 1255491
No longer depends on: 1255491

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5b2dc63c466a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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
Attachment #8728103 - Flags: approval-mozilla-beta?
Attachment #8728103 - Flags: approval-mozilla-aurora?
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
Attachment #8728103 - Flags: approval-mozilla-beta?
Attachment #8728103 - Flags: approval-mozilla-beta+
Attachment #8728103 - Flags: approval-mozilla-aurora?
Attachment #8728103 - Flags: approval-mozilla-aurora+
(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!
(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.
Flags: needinfo?(standard8)
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.
Flags: qe-verify+
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
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.
Flags: needinfo?(iulia.cristescu)
(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.
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.