335418 - Pause downloads when computer is about to go to sleep

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Andre Patry]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.2) Gecko/20060328 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.2) Gecko/20060328 Firefox/1.5.0.2

Data transfer (download) stop when the Mac goes into sleep mode. Had to restart the whole thing

Reproducible: Sometimes

Steps to Reproduce:
1. Start a big-enough download so you have time to.....(an .ISO is appropriate)
2. EITHER:
2a. Put the Mac into sleep mode voluntarily
2b. Wait for Power Management to do its thing.
3. Wake the Mac and check how the download is going.
Actual Results:  
I tried to reproduce the bug with 2a. The computer has been into sleep mode for less than 10 seconds and the download resumed itself so the stall might be a timeout from the remote server being caused by a sleeping computer/down HDD?

Expected Results:  
Download should keep going. PowerManagement shoud know there is activity and thus keep the disk spinning.

Comment 1

[OstGote!]

related Bug 338704

Comment 2

[Shawn Wilsher :sdwilsh]

We can actually register to get a notification when the mac is about to go to sleep.  We should try and pause downloads when this happens, and resume them when we wake up.  The docs indicate that there is a way to be notified on wakeup too, but doesn't mention what it is.

Josh - where would be the right place to add this code? (Ideally toolkit apps would want this, but I'm willing to go for just browser if need be).

Comment 3

[Josh Aas]

I don't know enough about client app architecture to say much on the subject. Our mac sleep/wake code is in nsToolkit.mm, probably easy to get it to notify on wakeup if it doesn't already.

Comment 4

[Shawn Wilsher :sdwilsh]

so, this is one of those easy things to do it looks like.  We dispatch  a notification to the observer service when the computer goes to sleep - "sleep_notification".  And when we wakeup, we get "wake_notification".  If someone wanted to do a patch with an xpcshell test to make sure we do the right stuff when those topics are dispatched we might just be able to get it in for 1.9.

I'll nominate it just in case drivers feel it's worth doing regardless.

Comment 5

[Ed Lee :Mardak]

Is this bug about using the observer notification or creating the notification somewhere as well as listening/using it in the download manager? It should be simple enough on the listener side -- we have a PauseAllDownloads and ResumeAllDownloads.

Comment 6

[Shawn Wilsher :sdwilsh]

The notifications are already there.  We just need to listen accordingly ;)

Comment 7

[Shawn Wilsher :sdwilsh]

Note: windows dispatches this as well, but linux doesn't seem to.  It's done in widget code for both of those, but linux doesn't seem to:
http://mxr.mozilla.org/seamonkey/search?string=sleep_notification

Comment 8

[Ed Lee :Mardak]

Do we need/want to resume on wake? Network connections are usually slow to come back.. so if we try to resume.. it might cause the download to fail...

Comment 9

[Shawn Wilsher :sdwilsh]

Fire a timer?

Comment 10

[Ed Lee :Mardak]

Attachment: v1

Pauses download on sleep as requested by the bug.

Comment 11

[Ed Lee :Mardak]

This build has the latest patch applied and should fix this issue:

https://build.mozilla.org/tryserver-builds/2008-03-16_18:[e-mail]/

Bug 403412 - Download Manager window title fails to clear/update upon download completion
Bug 335418 - Pause downloads when computer is about to go to sleep
Bug 414850 - "Clear List" in download manager should only remove visible downloads
Bug 420482 - Big discrepancy in ETA between Downloads Manager & Downloads status bar panel
Bug 414326 - Use DownloadUtils for software update downloads

Bug 393678 - location bar autocomplete should take word boundaries in account
Bug 407946 - emphasize all matching text in title and url, not just the first match in title and url
Bug 415403 - Show matches for all search words for location bar autocomplete
Bug 418257 - Show what part of which tags match for urlbar autocomplete
Bug 392143 - show keywords as url bar autocomplete choices
Bug 249468 - Add all bookmark keywords to location bar autocomplete drop-down list
Bug 395161 - make it possible to restrict the url bar autocomplete results to bookmarks, tags, or history entries
Bug 407204 - adjust the title and url text sizes
Bug 406257 - reduce the number of rows in url bar autocomplete from 10 to 6
Bug 420437 - Search and emphasize quoted strings with spaces
Bug 422491 - Optimize AwesomeBar if it finished searching and found fewer than maxResults

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 309782 [details] [diff] [review]
v1

I think we should fire a timer on wakeup (controlled by pref) to resume the downloads that we paused.

Comment 13

[Ed Lee :Mardak]

Attachment: v2

Now with more waking action.

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 310177 [details] [diff] [review]
v2

note:  comment 4 asked for a unit test to make sure that this works when those topics are dispatched.  Since this bug is only wanted-firefox3, I'm gonna need a test for this.

>+void // static
>+nsDownloadManager::ResumeOnWakeCallback(nsITimer *aTimer, void *aClosure)
>+{
>+  // Resume the downloads that were set to autoResume
>+  static_cast<nsDownloadManager *>(aClosure)->ResumeAllDownloads(PR_FALSE);
hey, ResumeAllDownloads returns something ;)

>+    // Wait a little bit before trying to resume to avoid resuming when network
>+    // connections haven't restarted yet
>+    mResumeOnWakeTimer = do_CreateInstance("@mozilla.org/timer;1");
>+    if (resumeOnWakeDelay >= 0 && mResumeOnWakeTimer)
>+      (void)mResumeOnWakeTimer->InitWithFuncCallback(ResumeOnWakeCallback,
>+        this, resumeOnWakeDelay, nsITimer::TYPE_ONE_SHOT);
while this is only one statement after the if, I'd like to see braces since it spans more than one line.

Comment 15

[Ed Lee :Mardak]

Attachment: v2.1

Added testcase based on that of bug 413985.

Comment 16

[Ed Lee :Mardak]

(In reply to comment #14)
> >+  static_cast<nsDownloadManager *>(aClosure)->ResumeAllDownloads(PR_FALSE);
> hey, ResumeAllDownloads returns something ;)
Split into cast then call (void)ed.

> >+    if (resumeOnWakeDelay >= 0 && mResumeOnWakeTimer)
> >+      (void)mResumeOnWakeTimer->InitWithFuncCallback(ResumeOnWakeCallback,
> >+        this, resumeOnWakeDelay, nsITimer::TYPE_ONE_SHOT);
> while this is only one statement after the if, I'd like to see braces since it
> spans more than one line.
Ok.

Comment 17

[Shawn Wilsher :sdwilsh]

Wouldn't it be better to use an xpcshell test here (don't have to worry about timeout - especially since we wait 10 seconds, and browser tests timeout in 15.

Comment 18

[Ed Lee :Mardak]

I changed the default delay to 1 for the testcase.

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 310277 [details] [diff] [review]
v2.1

I'd still prefer an xpcshell testcase over a browser one (easier to run, it's actually what we are encouraged to use, runs faster...), but r=sdwilsh regardless

Comment 20

[Ed Lee :Mardak]

Attachment: v2.2

Double the tests. Double the fun! Only check in the xpcshell one though?

Comment 21

[Shawn Wilsher :sdwilsh]

Yes - just the xpcshell test please

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Is a timeout the best we can do here? I thought we had some methods (from the offline browsing stuff) to determine when we're on or offline? dcamp?

Comment 23

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> Is a timeout the best we can do here? I thought we had some methods (from the
> offline browsing stuff) to determine when we're on or offline? dcamp?
I'm pretty sure we don't otherwise losing the internet connection would put us offline, and the download manager handles that.  This doesn't work on the mac now.  Might work on windows or linux - but certainly not the mac.

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

What happens if the 10s isn't long enough? Do we just leave the download in a paused state?

Comment 25

[Shawn Wilsher :sdwilsh]

I would expect the download to fail at that point.  Sadly we have no way of knowing if and when we have a connection available to us.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310332 [details] [diff] [review]
v2.2

a=beltzner, I guess this is better than nothing. I guess the user can restart and hope it resumes?

Comment 27

[Shawn Wilsher :sdwilsh]

We can increase the default number too - and it's a pref.  We don't provide UI for it, but probably could pretty easily.

Comment 28

[Ed Lee :Mardak]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.313; previous revision: 1.312
done
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.173; previous revision: 1.172
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.63; previous revision: 1.62
done
RCS file: /cvsroot/mozilla/toolkit/components/downloads/test/unit/test_sleep_wake.js,v
done
Checking in toolkit/components/downloads/test/unit/test_sleep_wake.js;
/cvsroot/mozilla/toolkit/components/downloads/test/unit/test_sleep_wake.js,v  <--  test_sleep_wake.js
initial revision: 1.1
done

Comment 29

[Justin Wood (:Callek)]

adding dev-doc-needed, for: https://developer.mozilla.org/En/Download_Manager_preferences