Closed Bug 623242 Opened 15 years ago Closed 14 years ago

Intermittent browser_bug553455.js | Should have seen the install fail - Got addon-progress-notification, expected addon-install-failed-notification, with or without a timeout, not Got 2 expected 1

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17
Tracking Flags:
Tracking Status
firefox15 --- fixed
firefox16 --- fixed
firefox-esr10 --- fixed

People

(Reporter: philor, Assigned: froydnj)

References

Details

(Keywords: intermittent-failure, Whiteboard: [qa-] fails in test_localfile)

Attachments

(3 files, 2 obsolete files)

Wait for popups to close before proceeding
18.93 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
wait for failure rather than notification
1.91 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
handle multiple notifications gracefully
4.02 KB, patch
mossop
: review+
lsblakk
: approval-mozilla-aurora+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1294242527.1294246844.3543.gz WINNT 5.2 mozilla-central debug test mochitest-other on 2011/01/05 07:48:47 s: win32-slave41 TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Should be only one notification TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Should have seen the install fail - Got addon-progress-notification, expected addon-install-failed-notification TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Should have seen the right message - Got Add-on downloading, expected This add-on could not be installed because it appears to be corrupt. TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Test part took 529ms TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Should be no active installs - Got 1, expected 0
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1294234863.1294239097.30706.gz WINNT 5.2 mozilla-central debug test mochitest-other on 2011/01/05 05:41:03 s: win32-slave39 TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Should be only one notification TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Should have seen the install fail - Got addon-progress-notification, expected addon-install-failed-notification TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Should have seen the right message - Got Add-on downloading, expected This add-on could not be installed because it appears to be corrupt. TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Test part took 565ms TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Should be no active installs - Got 1, expected 0
I'm not absolutely sure about comment 3, since it's on the new WinXP testers, where it could be XP-specific instead of a slightly different manifestation of this new thing.
Attached patch Prossible fix (obsolete) — — Splinter Review
This patch will fix most of failures. I can not reproduce this issue at least on my linux machine with this patch and the patch for bug 626998.
Attachment #506202 - Flags: review?
Attachment #506202 - Flags: review? → review?(dtownsend)
(In reply to comment #147) > Created attachment 506202 [details] [diff] [review] > Prossible fix > > This patch will fix most of failures. > > I can not reproduce this issue at least on my linux machine with this patch and > the patch for bug 626998. In which cases is the notification remaining open when it should have closed?
(In reply to comment #149) > (In reply to comment #147) > In which cases is the notification remaining open when it should have closed? Before running test_whitelisted_install, test_corrupt_file, test_incompatible. In the middle of wait_for_notification of test_corrupt_file and test_incompatible and others.
Comment on attachment 506202 [details] [diff] [review] Prossible fix Instead add code to those places to wait for the notification to close before proceeding.
Attachment #506202 - Flags: review?(dtownsend) → review-
(In reply to comment #153) > Comment on attachment 506202 [details] [diff] [review] > Prossible fix > > Instead add code to those places to wait for the notification to close before > proceeding. I understand what you want to do, but I did not get all of the cases which stays open the notification. So I did insert the code there.
Summary: Intermittent browser_bug553455.js | Should have seen the install fail - Got addon-progress-notification, expected addon-install-failed-notification etc., with or without a timeout → Intermittent browser_bug553455.js | Should have seen the install fail - Got addon-progress-notification, expected addon-install-failed-notification (or Got 2, expected 1 etc.), with or without a timeout
Hiroyuki has a point in that in some cases the popup isn't properly closed before we start a new test. This is mostly because we close the tab and then the notification starts to close at the next opportunity. It doesn't seem to actually cause any problems because I can reproduce that easily but not the test failures but it seems sensible to properly wait when necessary for those notifications to close before proceeding.
Attachment #512859 - Flags: review?(gavin.sharp)
Spun off the got 2 expected 1 failures into bug 634680 as they are a different issue.
Summary: Intermittent browser_bug553455.js | Should have seen the install fail - Got addon-progress-notification, expected addon-install-failed-notification (or Got 2, expected 1 etc.), with or without a timeout → Intermittent browser_bug553455.js | Should have seen the install fail - Got addon-progress-notification, expected addon-install-failed-notification, with or without a timeout
Whiteboard: [orange] → [orange] fails in test_localfile
Summary: Intermittent browser_bug553455.js | Should have seen the install fail - Got addon-progress-notification, expected addon-install-failed-notification, with or without a timeout → Intermittent browser_bug553455.js | Should have seen the install fail - Got addon-progress-notification, expected addon-install-failed-notification, with or without a timeout, not Got 2 expected 1
I think I can see this. This part of the test was never updated to handle the progress notification. That mostly doesn't matter though because we are dealing with a local file the "download" of it usually happens faster than it takes for the notification to display and so by the time it finishes opening the progress notification has already been replaced by the failure notification. Sometimes though the download is a little slow so we see the progress. Simple way around is to just wait for the failure event and then check that the notification shown is correct.
Attachment #506202 - Attachment is obsolete: true
Attachment #512925 - Flags: review?(gavin.sharp)
Note I think this patch will also fix the test_localfile part of the failure in bug 626998
Comment on attachment 512859 [details] [diff] [review] Wait for popups to close before proceeding This one may actually cause more problems though I don't know why
Attachment #512859 - Flags: review?(gavin.sharp)
Comment on attachment 512859 [details] [diff] [review] Wait for popups to close before proceeding Ok, this fails without the other patch in this bug so they both need to land
Attachment #512859 - Flags: review?(gavin.sharp)
Attachment #512925 - Flags: review?(gavin.sharp) → review+
Comment on attachment 512859 [details] [diff] [review] Wait for popups to close before proceeding Did you have to put the wait_for_notification_close calls before the removeTabs because the popuphidden is firing synchronously in those cases? I actually couldn't find any cases of nsXULPopupManager::HidePopup() being called with aAsynchronous=true that looked relevant to this test, so I'm skeptical that this is going to help...
Attachment #512859 - Flags: review?(gavin.sharp) → review+
Landed: http://hg.mozilla.org/mozilla-central/rev/be20deb20fd7 http://hg.mozilla.org/mozilla-central/rev/79bbb1de2577 Tentatively calling this fixed.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
No more failures in the last couple of days. Marking as verified fixed.
Assignee: nobody → dtownsend
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
So which version of the tests is Tracemonkey using? Hasn't the current state of mozilla-central been merged yet? Or why do we fail on tracemonkey only?
No, it hasn't - most of the time I star it, and star this as "mergeme," but every so often I fall asleep.
Happened on birch, which has the changesets in comment 275.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Not sure why this is still failing but not immediately working on it so open for anyone to take
Assignee: dtownsend → nobody
My changes in bug 715376 caused this test to perma-orange; rather than passing the buck, I thought I'd investigate. At least for my perma-orange, what happens is that when we run the executeSoon callback, there are two notifications pending: an addon-progress one and the one we actually expect to see, the addon-install-failed one. There was similar code to handle a situation like this in test_failed_security, so I chose that approach. It looks like other tests use wait_for_notification more liberally when waiting for addon-install-failed; I'm not all that familiar with the code, so I don't know which way is better. I do know this fixes my problems; I'd expect it to fix the intermittent oranges too.
Attachment #642645 - Flags: review?(dtownsend+bugmail)
Comment on attachment 642645 [details] [diff] [review] handle multiple notifications gracefully Review of attachment 642645 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/test/browser_bug553455.js @@ +70,5 @@ > } > }); > } > > +function wait_for_single_notification_with_handler(aCallback) { Should just be called wait_for_single_notification I think. @@ +82,5 @@ > + } > + > + aCallback(); > + wait_for_notification_close(runNextTest); > + gBrowser.removeTab(gBrowser.selectedTab); I don't think it makes sense to have these two lines in here.
Attachment #642645 - Flags: review?(dtownsend+bugmail) → review-
Version 2, with corrections.
Attachment #643913 - Flags: review?(dtownsend+bugmail)
Attachment #643913 - Flags: review?(dtownsend+bugmail) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/b669003ccc66
Assignee: nobody → nfroyd
Status: REOPENED → ASSIGNED
Target Milestone: mozilla2.0b12 → mozilla17
https://hg.mozilla.org/mozilla-central/rev/b669003ccc66
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 14 years ago
Resolution: --- → FIXED
Comment on attachment 643913 [details] [diff] [review] handle multiple notifications gracefully [Approval Request Comment] Bug caused by (feature/regressing bug #): Unknown User impact if declined: Higher rates of [orange] Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): Test-only orange fix. String or UUID changes made by this patch: None
Attachment #643913 - Flags: approval-mozilla-beta?
Attachment #643913 - Flags: approval-mozilla-aurora?
Comment on attachment 643913 [details] [diff] [review] handle multiple notifications gracefully test-only, orange fix, approving.
Attachment #643913 - Flags: approval-mozilla-beta?
Attachment #643913 - Flags: approval-mozilla-beta+
Attachment #643913 - Flags: approval-mozilla-aurora?
Attachment #643913 - Flags: approval-mozilla-aurora+
Attachment #642645 - Attachment is obsolete: true
(In reply to Nathan Froyd (:froydnj) from comment #602) > Created attachment 643913 [details] [diff] [review] > handle multiple notifications gracefully > > Version 2, with corrections. https://hg.mozilla.org/releases/mozilla-esr10/rev/a9422f3c2d8f
status-firefox-esr10: --- → fixed
Whiteboard: [orange] fails in test_localfile → [orange][qa-] fails in test_localfile
Whiteboard: [orange][qa-] fails in test_localfile → [qa-] fails in test_localfile
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: