1107405 - Doorhanger notifications don't show up only before Firefox closes

Status: RESOLVED INVALID

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

When we install addons for example the restart notification can't appear because the modal window is focused. The notification should appear when we close the modal and the main window get's focused but it doesn't until the browser shut's down.
https://www.youtube.com/watch?v=v85M-fiDk4A&feature=youtu.be

I've got this working when I removed the hack from Notifications which updates the notifications on tab focus in a setTimeout function.
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm?rev=297245ad4574#429

This reproduces only in an automated test, bug 1096178 comment 7.

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

Gavin, Gijs what do you think about this, could we use currentThread.dispatch instead of setTimeout(..., 0)?

Comment 2

[:Gijs (he/him)]

Comment on attachment 8532512 [details] [diff] [review]
patch v1.0

(In reply to Cosmin Malutan from comment #1)
> Created attachment 8532512 [details] [diff] [review]
> patch v1.0
> 
> Gavin, Gijs what do you think about this, could we use
> currentThread.dispatch instead of setTimeout(..., 0)?

No. These kinds of timing changes are pretty much always suspect, and especially in PopupNotifications.jsm I'd be loathe to change anything.

I don't really understand why this is implicitly blocking bug 989947, which was mac-only, while your video is from Linux. I'm pretty sure the Linux issue in the video has nothing to do with bug 989947.

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to :Gijs Kruitbosch from comment #2)
> No. These kinds of timing changes are pretty much always suspect, and
> especially in PopupNotifications.jsm I'd be loathe to change anything.
> 
> I don't really understand why this is implicitly blocking bug 989947, which
> was mac-only, while your video is from Linux. I'm pretty sure the Linux
> issue in the video has nothing to do with bug 989947.
Regarding this blocking bug 989947 I remember talking with Andrei about it and he said it's the same issue, I couldn't build it on OSX to check if it has the same root.

On the regression range from bug 1096178 comment 5, the window object from which we set the timeout changed. We could at least set the timout from the hiddenwindow, this also fixes the issue for us(Services.appShell.hiddenDOMWindow.setTimeout) otherwise we will have to keep the tests disabled.

Comment 4

[:Gijs (he/him)]

(In reply to Cosmin Malutan from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > No. These kinds of timing changes are pretty much always suspect, and
> > especially in PopupNotifications.jsm I'd be loathe to change anything.
> > 
> > I don't really understand why this is implicitly blocking bug 989947, which
> > was mac-only, while your video is from Linux. I'm pretty sure the Linux
> > issue in the video has nothing to do with bug 989947.
> Regarding this blocking bug 989947 I remember talking with Andrei about it
> and he said it's the same issue, I couldn't build it on OSX to check if it
> has the same root.

OK. The patch in bug 989947 is essentially OS X-only, so it'd be highly surprising if it changed anything on Linux.

> On the regression range from bug 1096178 comment 5, the window object from
> which we set the timeout changed.

Is that the same regression range as for this bug? Did anyone verify that?

> We could at least set the timout from the
> hiddenwindow, this also fixes the issue for
> us(Services.appShell.hiddenDOMWindow.setTimeout) otherwise we will have to
> keep the tests disabled.

It's not clear to me why this changes anything, and/or whether you're suggesting to change PopupNotifications.jsm, or the test.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to :Gijs Kruitbosch from comment #4)
> 
> OK. The patch in bug 989947 is essentially OS X-only, so it'd be highly
> surprising if it changed anything on Linux.
Okey so I made a wrong assumption because the bug has the same symptoms, the notification doesn't appear after we close a modal. I build it on OSX and it doesn't have the same cause. Sorry.

> > On the regression range from bug 1096178 comment 5, the window object from
> > which we set the timeout changed.
> 
> Is that the same regression range as for this bug? Did anyone verify that?
That was the bug for test failure, I filled a blocking bug under toolkit as requested bug 1096178 comment 17 

> > We could at least set the timout from the
> > hiddenwindow, this also fixes the issue for
> > us(Services.appShell.hiddenDOMWindow.setTimeout) otherwise we will have to
> > keep the tests disabled.
> 
> It's not clear to me why this changes anything, and/or whether you're
> suggesting to change PopupNotifications.jsm, or the test.
The suggestion is for PopupNotifications, setting the timeout from hiddenDOMWindow instead of this.window will dispatch the timeout under the same thread. This fixes the issue. If else, we could close this bug as wontfix and keep the test disabled or remove the check for the notification.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Being not able to handle notifications appropriately in our tests is somewhat a blocker for us. Given that Mozmill tests exist to exactly test ui specific behavior. 

So what I do not see right now is a very lightweight testcase which exactly shows this problem, and could be used also by developers to help us analyze the situation.

Also it's not clear to me which kind of notifications are affected. Are those all of them? Originally the problem was caused by a change to the web installer (bug 1084558). All in all the report as created here is very confusing in terms of what regressed what.

So I would propose to go back and really check all that again, I would propose to not use this bug for that. But only paste the final outcome in here. I can help with that.

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: testCase.js

A requested here is the test-case, for me it fails 1-2 times in 10 runs. I ran on Ubuntu 14.04 with latest Nightly.

Henrik, as discussed could you takeover?

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

This bug happens only with mozmill, so a real user won't be able to hit this, I've found a solution to avoid this edge case in our tests, and I explained it in bug 1096178 comment 28.

I'm closing this as INVALID