1096178 - Test failure 'Notification popup state has been opened' in /restartTests/testAddons_installMultipleExtensions/test1.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Andrei Eftimie]

Module:    testInstallMultipleExtensions
Test:      /restartTests/testAddons_installMultipleExtensions/test1.js    
Failure:   Notification popup state has been opened
Branches:  default
Platforms: OSX, Linux

Sometimes this message is recorded first:
"Panel is in the correct state - 'open' should equal 'closed'"

http://mozmill-daily.blargon7.com/#/functional/failure?app=Firefox&branch=All&platform=All&from=2014-11-04&test=%2FrestartTests%2FtestAddons_installMultipleExtensions%2Ftest1.js&func=testInstallMultipleExtensions

Looks like a regression.

I can't make any pattern out of it.
Of the 4th and on the 6th it failed on all OSX machines.
And then on the 8th and on the 9th it failed on Ubuntu.

(We switched on mozmill 2.0.9 on the 7th, maybe that has something to do with the failures switching from OSX to Linux?)

Comment 1

[Teodor Druta]

Reproduced this on Windows 
http://mozmill-crowd.blargon7.com/#/functional/report/43b0604030afed1afa485902878463ac

Comment 2

[Andrei Eftimie]

(In reply to Teodor Druta from comment #1)
> Reproduced this on Windows 
> http://mozmill-crowd.blargon7.com/#/functional/report/
> 43b0604030afed1afa485902878463ac

I also saw this on OSX. Once (out of 12+ runs).
Are you able to reproduce it constantly?

Try running the affected test in a loop. Maybe even reducing the executed code to get a testcase.

Comment 3

[Teodor Druta]

I replicated this issue on the staging XP machine today.
It is reproducible on any Firefox version and with any mozmill branch, so I highly doubt that this is a Firefox/Mozmill issue, rather a system issue.
The test failed in my investigation because the Firefox window didn't gain focus on start and opened in the foreground, the issue was to an opened system/installed application notification, unfortunately due to slow experience with the vnc connection I couldn't get to see what it was, and clicking it closed it.
I will continue to look more into this bug.

Comment 4

[Teodor Druta]

(In reply to Teodor Druta from comment #3)
> I replicated this issue on the staging XP machine today.
> It is reproducible on any Firefox version and with any mozmill branch, so I
> highly doubt that this is a Firefox/Mozmill issue, rather a system issue.
> The test failed in my investigation because the Firefox window didn't gain
> focus on start and opened in the foreground, the issue was to an opened
> system/installed application notification, unfortunately due to slow
> experience with the vnc connection I couldn't get to see what it was, and
> clicking it closed it.
> I will continue to look more into this bug.

I investigated this thoroughly on some Ubuntu machines and I came to the conclusion that the failures for Ubuntu and Mac OS X is of different nature than those from the Windows XP.
For Ubuntu it is indeed a regression range.

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ffa0f66db411&tochange=b6cd2dd85b26

Comment 5

[Teodor Druta]

I found the pushlog on fx-team:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=8047f8b92715&tochange=ea6f00d19dca

The regressor is Bug 1084558. 

Bug 1082764, Bug 1094312  also seems to be related with this issue.

The problem in our test is that when we click on the install button from the "Software Installation" dialog the "Restart firefox now" notification popup fails to open.

Comment 6

[Dave Townsend [:mossop]]

How can I reproduce this?

Comment 7

[Andrei Eftimie]

(In reply to Dave Townsend [:mossop] from comment #6)
> How can I reproduce this?

1. Clone the test repository from http://hg.mozilla.org/qa/mozmill-tests
2. Dowload latest mozmill-env for your system from https://mozqa.com/mozmill-env/
3. Activate the environment (./run or . bin/activate)
4. Run the test: 
> mozmill -m firefox/tests/functional/restartTests/testAddons_installMultipleExtensions/manifest.ini -b /Applications/FirefoxNightly.app/

Comment 8

[Dave Townsend [:mossop]]

I meant outside of the test. Is this just a test bug or a real bug that users can hit?

Comment 9

[Andrei Eftimie]

(In reply to Dave Townsend [:mossop] from comment #8)
> I meant outside of the test. Is this just a test bug or a real bug that
> users can hit?

This currently fails in our test. We don't yet have manual steps, but from the looks of how this fails we should probably be able to reproduce manually (unless there's some kind of race condition).

Teodor, please check if you are able to manually reproduce the issue, and include steps if so.
If not please attach a minimized testcase for mozmill.

Also from the available reports it looks like it's not failing intermittently: http://mozmill-daily.blargon7.com/#/functional/failure?app=Firefox&branch=All&platform=All&from=2014-11-04&test=%2FrestartTests%2FtestAddons_installMultipleExtensions%2Ftest1.js&func=testInstallMultipleExtensions

We should probably also disable the affected test on Nightly.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Teodor, please add a skip patch, it's failing a lot. This will also go in for next week to check.

Comment 11

[Teodor Druta]

Attachment: skipinstallmultipleext.patch

Added skip patch.

Comment 12

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8524594 [details] [diff] [review]
skipinstallmultipleext.patch

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

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/1e04124c084e (default)

Comment 13

[Andreea Matei [:AndreeaMatei]]

There's another test affected with the same error, we should check if it's related:
testAddons_InstallAddonWithoutEULA/test1.js
http://mozmill-daily.blargon7.com/#/remote/report/635fcffb06b246be47416301bd43d549

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: dump.txt

We have a race condition here, but it's a real bug.
The latest notification that we miss is shown in background, because at the moment notification get's instantiated, the focused window is not the one in which notification must be displayed, I added some dumps and I've got to isActiveWindow.
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm?rev=297245ad4574#319

Now I've got this far, but I don't know what would be the best approach to fix this. Personally I think we should get the notification even if the is not on the active window, otherwise it should be shown when we focus the window, but it doesn't. Also if the notification failed to open, it prevents us from opening new tabs.

Considering we show the notification on browser, and this is active, we shouldn't care so much about the window and show the notification anyway:


321     if (isActiveBrowser) {       
322       // show panel now
323       this._update(notifications, notification.anchorElement, true);
324
325       if (!isActiveWindow) {
326         // indicate attention and update the icon if necessary
327         if (!notification.dismissed) {
328           this.window.getAttention();
329         }
330       } 
331     }


Dave, what do you think? To reproduce this please follow the steps from comment 7.

Comment 15

[Cosmin Malutan, [:cosmin-malutan]]

So the pop-up should be updated right away, when the window gets the focus, but hit doesn't happen because it's in a setTimeout which get's called when the browser closes, even if it should execute right away. 
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm?rev=297245ad4574#429

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

I'll resume again what happens:

After we finish the installing of the addon, the notification which will require a restart should appear. Because the modal dialog is still focused(now it's closing), the notification is updated in background and it should appear right away when the window gets focused. This call happens in a setTimeout(... ,0) this should happen right away, but instead it get's called only when we close the browser. 

The solution here(which will not change the behavior) would be to use Services.tm.currentThread to dispatch the event.
We saw this in other tests too, so I expect this to fix all the "Notification state open" failures in our tests. 

I saw Gavin implemented this code, so I'll ask him for review.

Comment 17

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

Thanks Cosmin for this proposed fix. Can you please create a specific bug under toolkit for this patch? This bug is for the failing Mozmill test. Thanks.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

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

I don't understand how this would fix anything - these should be functionally equivalent, as you say. Why do you want to make this change?

Comment 19

[Cosmin Malutan, [:cosmin-malutan]]

The code as it's right(inside of setTimeout call) executes only when the window closes, I don't know exactly why but that's what I see when I log dumps and visually, the notification appears only when the window closes. This will allow us to re-enable lots of skipped test, due to intermittent notifications failures.

Gavin, do you agree with this change? Should I file a new bug or it will be pointless?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to Cosmin Malutan from comment #19)
> The code as it's right(inside of setTimeout call) executes only when the
> window closes

When what window closes?

Comment 21

[Cosmin Malutan, [:cosmin-malutan]]

The browser-chrome window, it appears just before the browser shuts down, that's after our test failed.
https://www.youtube.com/watch?v=v85M-fiDk4A&feature=youtu.be

Comment 22

[Teodor Druta]

Attachment: skipinstallmulext.patch

It seeems that there is an issue skipping manifest.ini directly, updated the patch to skip directly the tests.

Comment 23

[Teodor Druta]

Attachment: skipinstallmulext.patch

After removing the changeset, there are rejections, fixed the patch.

Comment 24

[Teodor Druta]

Attachment: skipinstallmulext.patch

After landing the new patch from Bug 1088561, the patch is not applying anymore, fixed rejections.

Comment 25

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8535643 [details] [diff] [review]
skipinstallmulext.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/cffd6c8a8fba (default)

This will cause bug 1110837 to happen on aurora too if we move testChangeTheme from there and this test remains first.

Comment 26

[Daniela Domnici]

Attachment: unskipInstallMultipleExtensions

I have created an unskip patch for these tests because the Bug 1108994 will fix this issue.

Comment 27

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8536497 [details] [diff] [review]
unskipInstallMultipleExtensions

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

This is blocked by the other bug, please update the Depends on here. Also the disabled will be at that point in the testAddons folder manifest, so you need to unblock from that bug first.

Comment 28

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 19-12-irclog.txt

To resume what was discussed here, we fail in waiting for a notification after we install two extensions, please see the video from comment 21.
The waitFor is inside of dialog handler, if I move it outside of the handler the test passes, as discussed on irc on 19/12/2014 we interfere with the browser in our tests and our waitFor function enforce the Firefox to run synchronously, that's why we see the notification only when the Firefox closes. 

As a solution I just had to move the waitForNotification call outside of modal handler.
http://mozmill-crowd.blargon7.com/#/functional/report/c922e10f6ac9f80d9cffdca9cf057e72

Comment 29

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

Here is the fix patch, see the report above, also this was failing 100% on ubuntu, so this definitely fixes the test.

Comment 30

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

This is the correct patch

Comment 31

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1

I fixed a nit, I call callback directly in waitForNotification

Comment 32

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

I re-arranged a bit the code.
http://mozmill-crowd.blargon7.com/#/functional/report/c922e10f6ac9f80d9cffdca9cf31aeef

Comment 33

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8558483 [details] [diff] [review]
patch v2.1

Andreea this is the working patch, the latest one has intermittent failures because we register the listeners when the popoup is already opened

Comment 34

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8558483 [details] [diff] [review]
patch v2.1

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

It no longer applies in the manifest, please update. Otherwise tests well.

Comment 35

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.2

Thanks

Comment 36

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8559601 [details] [diff] [review]
patch v2.2

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

https://hg.mozilla.org/qa/mozmill-tests/rev/7d68f2abf7c3 (default)

Comment 37

[Cosmin Malutan, [:cosmin-malutan]]

Andreea, this ran perfectly on default for 4 days, let's backport it down to release.

Comment 38

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.2 beta

The previous patch applies on aurora, this is the patch for beta.

Comment 39

[Andreea Matei [:AndreeaMatei]]

https://hg.mozilla.org/qa/mozmill-tests/rev/c76cfa6a90d2 (aurora)

Comment 40

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8561278 [details] [diff] [review]
patch v2.2 beta

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

https://hg.mozilla.org/qa/mozmill-tests/rev/3ed735e6f581 (beta)

Comment 41

[Andreea Matei [:AndreeaMatei]]

I see release is skipped too, the patch on beta doesn't apply to it.

Comment 42

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.2 release

This is patch for release.

Comment 43

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8561902 [details] [diff] [review]
patch v2.2 release

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

https://hg.mozilla.org/qa/mozmill-tests/rev/f49441edc75c (release)

Comment 44

[Andreea Matei [:AndreeaMatei]]

ESR31 is also affected:
http://mozmill-daily.blargon7.com/#/functional/failure?app=Firefox&branch=31&platform=All&from=2014-11-04&to=2015-02-10&test=%2FrestartTests%2FtestAddons_installMultipleExtensions%2Ftest1.js&func=testInstallMultipleExtensions

Comment 45

[Cosmin Malutan, [:cosmin-malutan]]

For esr we should reopen bug 1088551 and backport that fix too.

Comment 46

[Andreea Matei [:AndreeaMatei]]

Lets do that please.

Comment 47

[Cosmin Malutan, [:cosmin-malutan]]

Andreea the esr is not affected this bug was 100% reproducible, but I can't reproduce it neither locally or on a production node. Also it failed only on Ubuntu 14.04, this bug affected all both Ubuntu versions and also OSX. I expect to be a display size issue. If it fails again I suggest we open a new bug for that, the issue for this specific bug was clearly fixed.