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

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: andrei, Assigned: cosmin-malutan)

Tracking

({intermittent-failure, regression})

unspecified
intermittent-failure, regression
Dependency tree / graph

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed, firefox37 fixed, firefox38 fixed, firefox-esr31 unaffected)

Details

(Whiteboard: [mozmill-test-failure][mozmill-test-skipped][sprint], URL)

Attachments

(4 attachments, 11 obsolete attachments)

3.92 KB, text/plain
Details
2.99 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
3.09 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
3.07 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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?)

Updated

4 years ago
Assignee: nobody → teodor.druta
Status: NEW → ASSIGNED
(Reporter)

Comment 2

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

4 years ago
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

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

4 years ago
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.
Blocks: 1084558
Flags: needinfo?(dtownsend+bugmail)
Keywords: regressionwindow-wanted
How can I reproduce this?
Flags: needinfo?(dtownsend+bugmail)
(Reporter)

Comment 7

4 years ago
(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/
I meant outside of the test. Is this just a test bug or a real bug that users can hit?
(Reporter)

Comment 9

4 years ago
(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.
Flags: needinfo?(teodor.druta)
Priority: P2 → P1
Teodor, please add a skip patch, it's failing a lot. This will also go in for next week to check.

Comment 11

4 years ago
Created attachment 8524594 [details] [diff] [review]
skipinstallmultipleext.patch

Added skip patch.
Flags: needinfo?(teodor.druta)
Attachment #8524594 - Flags: checkin?(andreea.matei)
Attachment #8524594 - Flags: checkin?(andreea.matei) → checkin+
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)
status-firefox36: affected → disabled
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-test-skipped]
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
(Assignee)

Comment 14

4 years ago
Created attachment 8525892 [details]
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.
Flags: needinfo?(dtownsend+bugmail)
Assignee: teodor.druta → cosmin.malutan
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure][mozmill-test-skipped][sprint]
(Assignee)

Comment 15

4 years ago
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
Flags: needinfo?(dtownsend+bugmail)
(Assignee)

Comment 16

4 years ago
Created attachment 8528870 [details] [diff] [review]
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.
Attachment #8528870 - Flags: review?(gavin.sharp)
Attachment #8528870 - Flags: feedback?(andrei.eftimie)
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 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?
(Assignee)

Comment 19

4 years ago
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?
Flags: needinfo?(gavin.sharp)
(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?
Flags: needinfo?(gavin.sharp)
(Assignee)

Comment 21

4 years ago
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
(Assignee)

Updated

4 years ago
Depends on: 1107405
(Assignee)

Updated

4 years ago
Attachment #8528870 - Attachment is obsolete: true
Attachment #8528870 - Flags: review?(gavin.sharp)
Attachment #8528870 - Flags: feedback?(andrei)

Comment 22

4 years ago
Created attachment 8535633 [details] [diff] [review]
skipinstallmulext.patch

It seeems that there is an issue skipping manifest.ini directly, updated the patch to skip directly the tests.
Attachment #8524594 - Attachment is obsolete: true
Attachment #8535633 - Flags: review?(andreea.matei)

Comment 23

4 years ago
Created attachment 8535640 [details] [diff] [review]
skipinstallmulext.patch

After removing the changeset, there are rejections, fixed the patch.
Attachment #8535633 - Attachment is obsolete: true
Attachment #8535633 - Flags: review?(andreea.matei)
Attachment #8535640 - Flags: review?(andreea.matei)

Comment 24

4 years ago
Created attachment 8535643 [details] [diff] [review]
skipinstallmulext.patch

After landing the new patch from Bug 1088561, the patch is not applying anymore, fixed rejections.
Attachment #8535640 - Attachment is obsolete: true
Attachment #8535640 - Flags: review?(andreea.matei)
Attachment #8535643 - Flags: review?(andreea.matei)
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.
Attachment #8535643 - Flags: review?(andreea.matei) → review+

Comment 26

4 years ago
Created attachment 8536497 [details] [diff] [review]
unskipInstallMultipleExtensions

I have created an unskip patch for these tests because the Bug 1108994 will fix this issue.
Attachment #8525892 - Attachment is obsolete: true
Attachment #8535643 - Attachment is obsolete: true
Attachment #8536497 - Flags: review?(mihaela.velimiroviciu)
Attachment #8536497 - Flags: review?(andreea.matei)
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.
Attachment #8536497 - Flags: review?(mihaela.velimiroviciu)
Attachment #8536497 - Flags: review?(andreea.matei)
Attachment #8536497 - Flags: review-

Updated

4 years ago
Depends on: 1108994
No longer depends on: 1107405

Updated

4 years ago
Depends on: 1107405
Whiteboard: [mozmill-test-failure][mozmill-test-skipped][sprint] → [mozmill-test-failure][mozmill-test-skipped][sprint][blocked by 1107405]
(Assignee)

Comment 28

4 years ago
Created attachment 8558448 [details]
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
(Assignee)

Comment 29

4 years ago
Created attachment 8558449 [details] [diff] [review]
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.
Attachment #8558449 - Flags: review?(andreea.matei)
(Assignee)

Comment 30

4 years ago
Created attachment 8558450 [details] [diff] [review]
patch v1.0

This is the correct patch
Attachment #8558449 - Attachment is obsolete: true
Attachment #8558449 - Flags: review?(andreea.matei)
Attachment #8558450 - Flags: review?(andreea.matei)
(Assignee)

Comment 31

4 years ago
Created attachment 8558483 [details] [diff] [review]
patch v2.1

I fixed a nit, I call callback directly in waitForNotification
Attachment #8558450 - Attachment is obsolete: true
Attachment #8558450 - Flags: review?(andreea.matei)
Attachment #8558483 - Flags: review?(andreea.matei)
(Assignee)

Updated

4 years ago
Attachment #8558483 - Flags: review?(mihaela.velimiroviciu)
(Assignee)

Comment 32

4 years ago
Created attachment 8558498 [details] [diff] [review]
patch v2.0

I re-arranged a bit the code.
http://mozmill-crowd.blargon7.com/#/functional/report/c922e10f6ac9f80d9cffdca9cf31aeef
Attachment #8558483 - Attachment is obsolete: true
Attachment #8558483 - Flags: review?(mihaela.velimiroviciu)
Attachment #8558483 - Flags: review?(andreea.matei)
Attachment #8558498 - Flags: review?(mihaela.velimiroviciu)
Attachment #8558498 - Flags: review?(andreea.matei)
Attachment #8558498 - Flags: review?(mihaela.velimiroviciu) → review+
(Assignee)

Comment 33

4 years ago
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
Attachment #8558483 - Attachment description: patch v1.1 → patch v2.1
Attachment #8558483 - Attachment is obsolete: false
Attachment #8558483 - Flags: review?(andreea.matei)
(Assignee)

Updated

4 years ago
Attachment #8558498 - Attachment is obsolete: true
Attachment #8558498 - Flags: review?(andreea.matei)
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.
Attachment #8558483 - Flags: review?(andreea.matei) → review+
(Assignee)

Comment 35

4 years ago
Created attachment 8559601 [details] [diff] [review]
patch v2.2

Thanks
Attachment #8558483 - Attachment is obsolete: true
Attachment #8559601 - Flags: review?(andreea.matei)
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)
Attachment #8559601 - Flags: review?(andreea.matei) → review+
status-firefox37: --- → disabled
status-firefox38: --- → fixed
Whiteboard: [mozmill-test-failure][mozmill-test-skipped][sprint][blocked by 1107405] → [mozmill-test-failure][mozmill-test-skipped][sprint]
(Assignee)

Comment 37

4 years ago
Andreea, this ran perfectly on default for 4 days, let's backport it down to release.
Flags: needinfo?(andreea.matei)
(Assignee)

Comment 38

4 years ago
Created attachment 8561278 [details] [diff] [review]
patch v2.2 beta

The previous patch applies on aurora, this is the patch for beta.
Attachment #8536497 - Attachment is obsolete: true
Attachment #8561278 - Flags: review?(andreea.matei)
https://hg.mozilla.org/qa/mozmill-tests/rev/c76cfa6a90d2 (aurora)
status-firefox37: disabled → fixed
Flags: needinfo?(andreea.matei)
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)
Attachment #8561278 - Flags: review?(andreea.matei) → review+
I see release is skipped too, the patch on beta doesn't apply to it.
status-firefox36: disabled → fixed
(Assignee)

Comment 42

4 years ago
Created attachment 8561902 [details] [diff] [review]
patch v2.2 release

This is patch for release.
Attachment #8561902 - Flags: review?(andreea.matei)
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)
Attachment #8561902 - Flags: review?(andreea.matei) → review+
(Assignee)

Comment 45

4 years ago
For esr we should reopen bug 1088551 and backport that fix too.
Lets do that please.
(Assignee)

Comment 47

4 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox-esr31: affected → unaffected
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.