887425 - BackgroundFileSaver should support appending to an existing file

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[:Paolo Amadini]

In the new Downloads API, we need the ability to resume partially downloaded
files without working on the main thread, and still support hash computation.

Comment 1

[:Paolo Amadini]

Attachment: The patch

This adds the option to enable append, similarly to how we enable hashing, and
also adds one more test for some of the hashing code that was touched.

Patrick, do you know who could review and superreview this?

Comment 2

[Patrick McManus [:mcmanus]]

jason, have you ever worked in this neighborhood? I've only passed through briefly on the way to somewhere else myself?

If you're not the right person for it let me know and I'll do my best.

Comment 3

[Jason Duell]

I've never looked at this code.  Looks like only biesi has reviewed this stuff.  I'd take it but I'm on PTO soon.

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 767933 [details] [diff] [review]
The patch

Review of attachment 767933 [details] [diff] [review]:
-----------------------------------------------------------------

ask biesi for sr?

I'm afraid my review here isn't very thorough - but I learned a bit about the code..

::: netwerk/base/public/nsIBackgroundFileSaver.idl
@@ +81,5 @@
> +   *
> +   * @remarks This must be set on the main thread before the first call to
> +   *          setTarget.
> +   */
> +  void enableAppend();

bump the uuid in the idl

::: netwerk/base/src/BackgroundFileSaver.cpp
@@ +166,5 @@
>  // Called on the control thread.
>  NS_IMETHODIMP
> +BackgroundFileSaver::EnableAppend()
> +{
> +  {

moz_assert(ns_ismainthread())
useless layer of scope braces

Comment 5

[:Paolo Amadini]

Attachment: Updated patch

Christian, do you have some time for reviewing the IDL here? I may also ask
Gavin if you can't look into this now.

(In reply to Patrick McManus [:mcmanus] from comment #4)
> I'm afraid my review here isn't very thorough - but I learned a bit about
> the code..

Thanks for the prompt review!

Comment 6

[:Paolo Amadini]

Thanks for the quick superreview!

Before landing, I have to figure out the cause of a test failure that happens on
Linux only in test_empty_hash, where .fileSize doesn't find the file:

https://tbpl.mozilla.org/?tree=Try&rev=6962e8f5e3b9

It seems that the file isn't created, but strangely, this doesn't happen when the
individual test is run locally on my Linux Mint 13 system.

Anyone has clues on why the empty file isn't created there, and why on my system
I have different behavior than the Ubuntu system on the tryserver?

Comment 7

[:Paolo Amadini]

(In reply to Paolo Amadini [:paolo] from comment #6)
> It seems that the file isn't created, but strangely, this doesn't happen
> when the individual test is run locally on my Linux Mint 13 system.

This was caused by a race condition, fixed in this patch.

I'll land this in a day or two, feel free to take a look at the solution in the
meantime if you want, and let me know if you have anything to note.

Comment 8

[:Paolo Amadini]

Attachment: Fix race condition

Comment 9

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1603c51eb48a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1603c51eb48a

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130911030258

Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Build ID: 20130912004004

Verified that paused downloads (by using "Pause" option or caused by browser quit) are resumed successfully, and continue where they were left off.
Marking VERIFIED