Closed Bug 582315 Opened 9 years ago Closed 9 years ago

intermittent orange in test_taskbarprogress_downloadstates.xul and test_taskbarprogress_service.xul

Categories

(Toolkit :: Downloads API, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b4

People

(Reporter: dbaron, Assigned: Felipe)

References

()

Details

Attachments

(2 files)

The new Windows 7 tinderboxes are showing intermittent orange of the following form multiple times per day (although less than 20% of the time):


3523 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_taskbarprogress_downloadstates.xul | Correct paused state
3524 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_taskbarprogress_downloadstates.xul | Correct paused state
3525 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_taskbarprogress_downloadstates.xul | There shouldn't be negative download counts
3528 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_taskbarprogress_service.xul | Correct downloading state
3529 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_taskbarprogress_service.xul | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDownloadManager.resumeDownload]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://mochikit/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_taskbarprogress_downloadstates.xul :: testSteps :: line 145"  data: no] at :0


These tests are only run on Windows 7.


Two most recent occurrences are:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280225301.1280226923.32613.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280221659.1280223206.13998.gz


Felipe, could you have a look at this and see if you can figure out what's going on?
I'll take a look dbaron. Is this the first time these tests are being run, or were there previous win7 tinderboxes where this orange doesn't show up?
Attached patch Fix attemptSplinter Review
I've been able to reproduce it here. All the steps of the test are done after yielding code and re-entering after an executeSoon.  Turns out, it's possible to call dm.pauseDownload(), yield, receive the paused download state change, but the download can still finish, even without explicitly resuming.

I changed the pauseDownload call to inside the downloadstate listener that gets the "new download" notification (which will happen sooner, without an yield in-between), and the intermittent failure doesn't happen locally anymore. 

dbaron, if this test fix doesn't need review (I'm never sure), could you push it to see if fixes the problem on tinderbox?

I believe the failure on test_taskbarprogress_service.xul is just a consequence of a bad state left when test_taskbarprogress_downloadstates.xul fails.
Assignee: nobody → felipc
Status: NEW → ASSIGNED
We're seeing this failure on more than half of the runs:

3572 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_taskbarprogress_downloadstates.xul | Correct paused state

and on one run it happened twice.

The other failures seem to be fixed, though.
Got it. The paused download is paused at various progress levels, sometimes as 100% (but still active, before it transitions to finished). However, a 100% paused download is not represented as STATE_PAUSED in the taskbar, but rather as STATE_NO_PROGRESS, due to this check in DownloadTaskbarProgress.jsm

       while (downloads.hasMoreElements()) {
         let download = downloads.getNext().QueryInterface(Ci.nsIDownload);
         // Only set values if we actually know the download size
>        if (download.percentComplete < 100 && download.size > 0) {
           totalSize += download.size;
           totalTransferred += download.amountTransferred;
         }
         // We might need to display a paused state, so track this
         if (download.state == this._dm.DOWNLOAD_PAUSED) {
           numPaused++;
         }
       }

We can either a) remove the download.percentComplete < 100 check, or b) adjust the test to accept STATE_NO_PROGRESS for paused downloads.

Sid, do you remember why that check exists? Which of the two above do you suggest doing?
Urgh, according to <https://developer.mozilla.org/en/nsIDownload#Attributes> that check is all wrong. No idea why I messed up so badly, sorry :( We should just check whether percentComplete is -1.
(In reply to comment #7)
> Urgh, according to <https://developer.mozilla.org/en/nsIDownload#Attributes>
> that check is all wrong. No idea why I messed up so badly, sorry :( We should
> just check whether percentComplete is -1.
hey, I missed it in review too :(
Attached patch FixSplinter Review
Like so.

The indeterminate progress that is displayed when a download is being scanned for viruses depended on that check, so just fixing the check was not enough. I've made the necessary adjustments.
Attachment #462966 - Flags: review?(sdwilsh)
Comment on attachment 462966 [details] [diff] [review]
Fix

r=sdwilsh for awesome
Attachment #462966 - Flags: review?(sdwilsh) → review+
Comment on attachment 462966 [details] [diff] [review]
Fix

a=sdwilsh on this very trivial, very simple fix to some random orange (really, it's a correctness fix but unlikely to come up for most people).
Attachment #462966 - Flags: approval2.0+
Passed four runs in a row, I'd guess it's fixed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
You need to log in before you can comment on or make changes to this bug.