1261344 - Fix race condition with the state of Download objects when Application Reputation blocking occurs

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

DownloadSaver objects should not change the state of Download objects directly. This bug fixes a possible race condition related to that issue when Application Reputation blocking occurs.

Comment 1

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1261344 - Fix race condition with the state of Download objects when Application Reputation blocking occurs. r=mak

Review commit: https://reviewboard.mozilla.org/r/43783/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43783/

Comment 2

[:Paolo Amadini]

This race condition occurred to me a while ago when looking at an unrelated bug about the placeholder file.

At some point we might want to refactor this logic to use a DownloadAttempt object or something similar, where Download provides a view onto the DownloadAttempt state thus making the filtering for past attempts unnecessary, but it's a major change and I'd leave that for the future.

It also occurs to me that the OS.File accesses we currently do in the Download object for blocking and unblocking should be ideally delegated to the DownloadSaver or the DownloadTarget. The Download object should only deal with the state handling and with making sure the DownloadSaver does not race with itself.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8737171 [details]
MozReview Request: Bug 1261344 - Fix race condition with the state of Download objects when Application Reputation blocking occurs. r=mak

https://reviewboard.mozilla.org/r/43783/#review41533

LGTM

::: toolkit/components/jsdownloads/src/DownloadCore.jsm:444
(Diff revision 1)
> +      }
> +
> +      if ("hasBlockedData" in aOptions) {
> +        this.hasBlockedData = aOptions.hasBlockedData;
> +        changeMade = true;
> +      }

could also be rewritten as a loop through ["progress", "hasPartialData", "hasBlockedData"]
Maybe also including contentType?

Actually, is there a specific reason contentType checks if there's a difference in the value before setting changeMade while these don't?

Comment 4

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #3)
> Actually, is there a specific reason contentType checks if there's a
> difference in the value before setting changeMade while these don't?

No, it was just not needed in the specific case, but I've added the checks when rewriting as a loop.

Comment 5

[:Paolo Amadini]

Comment on attachment 8737171 [details]
MozReview Request: Bug 1261344 - Fix race condition with the state of Download objects when Application Reputation blocking occurs. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43783/diff/1-2/

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/29d7a174bdb4

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/29d7a174bdb4