Closed
Bug 1096178
Opened 10 years ago
Closed 10 years ago
Test failure 'Notification popup state has been opened' in /restartTests/testAddons_installMultipleExtensions/test1.js
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(firefox35 fixed, firefox36 fixed, firefox37 fixed, firefox38 fixed, firefox-esr31 unaffected)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox35 | --- | fixed |
firefox36 | --- | fixed |
firefox37 | --- | fixed |
firefox38 | --- | fixed |
firefox-esr31 | --- | unaffected |
People
(Reporter: andrei, Assigned: cosmin-malutan)
References
()
Details
(Keywords: intermittent-failure, regression, Whiteboard: [mozmill-test-failure][mozmill-test-skipped][sprint])
Attachments
(4 files, 11 obsolete files)
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 |
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•10 years ago
|
Assignee: nobody → teodor.druta
Status: NEW → ASSIGNED
Comment 1•10 years ago
|
||
Reproduced this on Windows
http://mozmill-crowd.blargon7.com/#/functional/report/43b0604030afed1afa485902878463ac
Reporter | ||
Comment 2•10 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•10 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•10 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•10 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.
Reporter | ||
Comment 7•10 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/
Comment 8•10 years ago
|
||
I meant outside of the test. Is this just a test bug or a real bug that users can hit?
Reporter | ||
Comment 9•10 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
Comment 10•10 years ago
|
||
Teodor, please add a skip patch, it's failing a lot. This will also go in for next week to check.
Comment 11•10 years ago
|
||
Added skip patch.
Flags: needinfo?(teodor.druta)
Attachment #8524594 -
Flags: checkin?(andreea.matei)
Updated•10 years ago
|
Attachment #8524594 -
Flags: checkin?(andreea.matei) → checkin+
Comment 12•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][mozmill-test-skipped]
Comment 13•10 years ago
|
||
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•10 years ago
|
||
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)
Updated•10 years ago
|
Assignee: teodor.druta → cosmin.malutan
Whiteboard: [mozmill-test-failure][mozmill-test-skipped] → [mozmill-test-failure][mozmill-test-skipped][sprint]
Assignee | ||
Comment 15•10 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•10 years ago
|
||
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)
Comment 17•10 years ago
|
||
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•10 years ago
|
||
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•10 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)
Comment 20•10 years ago
|
||
(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•10 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•10 years ago
|
Attachment #8528870 -
Attachment is obsolete: true
Attachment #8528870 -
Flags: review?(gavin.sharp)
Attachment #8528870 -
Flags: feedback?(andrei)
Comment 22•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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 25•10 years ago
|
||
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•10 years ago
|
||
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 27•10 years ago
|
||
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•10 years ago
|
Updated•10 years ago
|
Whiteboard: [mozmill-test-failure][mozmill-test-skipped][sprint] → [mozmill-test-failure][mozmill-test-skipped][sprint][blocked by 1107405]
Assignee | ||
Comment 28•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
Attachment #8558483 -
Flags: review?(mihaela.velimiroviciu)
Assignee | ||
Comment 32•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8558498 -
Flags: review?(mihaela.velimiroviciu) → review+
Assignee | ||
Comment 33•10 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•10 years ago
|
Attachment #8558498 -
Attachment is obsolete: true
Attachment #8558498 -
Flags: review?(andreea.matei)
Comment 34•10 years ago
|
||
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•10 years ago
|
||
Thanks
Attachment #8558483 -
Attachment is obsolete: true
Attachment #8559601 -
Flags: review?(andreea.matei)
Comment 36•10 years ago
|
||
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+
Updated•10 years ago
|
status-firefox37:
--- → disabled
status-firefox38:
--- → fixed
Updated•10 years ago
|
Whiteboard: [mozmill-test-failure][mozmill-test-skipped][sprint][blocked by 1107405] → [mozmill-test-failure][mozmill-test-skipped][sprint]
Assignee | ||
Comment 37•10 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•10 years ago
|
||
The previous patch applies on aurora, this is the patch for beta.
Attachment #8536497 -
Attachment is obsolete: true
Attachment #8561278 -
Flags: review?(andreea.matei)
Comment 39•10 years ago
|
||
Flags: needinfo?(andreea.matei)
Comment 40•10 years ago
|
||
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+
Comment 41•10 years ago
|
||
I see release is skipped too, the patch on beta doesn't apply to it.
Assignee | ||
Comment 42•10 years ago
|
||
This is patch for release.
Attachment #8561902 -
Flags: review?(andreea.matei)
Comment 43•10 years ago
|
||
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+
Comment 44•10 years ago
|
||
status-firefox35:
--- → fixed
status-firefox-esr31:
--- → affected
Assignee | ||
Comment 45•10 years ago
|
||
For esr we should reopen bug 1088551 and backport that fix too.
Comment 46•10 years ago
|
||
Lets do that please.
Assignee | ||
Comment 47•10 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
Closed: 10 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•