582315 - intermittent orange in test_taskbarprogress_downloadstates.xul and test_taskbarprogress_service.xul

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 1

[:Felipe Gomes (needinfo for replies!)]

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?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280211743.1280213314.8453.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280206653.1280208204.21360.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280201758.1280203367.3241.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280194161.1280195788.7757.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280190273.1280193163.28817.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280188926.1280190575.17959.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1280185044.1280186603.1000.gz

So, actually, I take back the "less than 20% of the time" thing.


There weren't previous Win7 tinderboxes; the current ones have been running for at least a few weeks, gradually becoming less orange as people fix the existing oranges.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Fix attempt

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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

landed: http://hg.mozilla.org/mozilla-central/rev/11ef53df4a4d

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

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?

Comment 7

[Rain [:rain1] (inactive)]

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.

Comment 8

[Shawn Wilsher :sdwilsh]

(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 :(

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: Fix

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.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 462966 [details] [diff] [review]
Fix

r=sdwilsh for awesome

Comment 11

[Shawn Wilsher :sdwilsh]

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).

Comment 12

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/e30d7181cc4a

Comment 13

[Phil Ringnalda (:philor)]

Passed four runs in a row, I'd guess it's fixed.