Closed Bug 1261344 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file)

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.
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 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?
Attachment #8737171 - Flags: review?(mak77) → review+
(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 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/
https://hg.mozilla.org/mozilla-central/rev/29d7a174bdb4
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.