989419 - Remove main-thread IO from background update of {profile}\blocklist.xml

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[:Irving Reid (No longer working on Firefox)]

+++ This bug was initially created as a clone of Bug #988304 +++

{profile}\blocklist.xml is downloaded and replaced in the background, using sync I/O to save and re-read the file. The save and re-read should be done using async I/O.

Comment 1

[:Irving Reid (No longer working on Firefox)]

Blair, do you think it's worth going ahead with this before you get around to rewriting the service (bug bug 782261)?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ugh, I still have no timeframe for when that rewrite may happen (which isn't so much as a rewrite but a re-envisioning).

Reading through bug 988304 comment 2, you think this can be done without external API changes? If so, then that seems like we should just go ahead with this.

Comment 3

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Remove main-thread IO from background update of {profile}\blocklist.xml, v1

Comment 4

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8402732 [details] [diff] [review]
Remove main-thread IO from background update of {profile}\blocklist.xml, v1

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

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +599,2 @@
>  
> +    yield this._loadBlocklistFromFileAsync(path);

Rather than re-reading the blocklist from disk here, why not just pass 'request.responseText' to _loadBlocklistFromString()?

@@ +708,5 @@
> +    let text;
> +    try{
> +      text = yield OS.File.read(path, { encoding: "utf-8"})
> +    } catch (e if e instanceof OS.File.Error && e.becauseNoSuchFile) {
> +      LOG("Blocklist::_loadBlocklistFromFile: XML File does not exist");

Please put the file name in the log message when a file operation fails.

@@ +735,2 @@
>      fileStream.init(file, FileUtils.MODE_RDONLY, FileUtils.PERMS_FILE, 0);
> +    cstream.init(fileStream, "UTF-8", 0, 0);

The old code didn't have a try/catch around fileStream.init(...), but I think it needs one.

Comment 5

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Remove main-thread IO from background update of {profile}\blocklist.xml, v2

Comment 6

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8403257 [details] [diff] [review]
Remove main-thread IO from background update of {profile}\blocklist.xml, v2

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

r+  after (a) the tmpPath change and (b) Blair's feedback on handling failure writing to file.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +589,5 @@
> +
> +    let path = OS.Path.join(OS.Constants.Path.profileDir, FILE_BLOCKLIST);
> +    let encoder = new TextEncoder();
> +    let array = encoder.encode(request.responseText);
> +    yield OS.File.writeAtomic(path, array);

I would add options {tmpPath:...} to this call, to do a safer write & rename rather than writing directly to the file. A slight behaviour change from the existing code, but worth doing in my opinion.

Question for the module owner: Do we want to use the new blocklist in memory, even if saving it to file fails? If so, we probably want to move the _loadBlocklistFromString to before the OS.File.writeAtomic, and probably exception-catch the writeAtomic call.

Comment 7

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Remove main-thread IO from background update of {profile}\blocklist.xml, v3

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Irving Reid from comment #6)
> Question for the module owner: Do we want to use the new blocklist in
> memory, even if saving it to file fails? If so, we probably want to move the
> _loadBlocklistFromString to before the OS.File.writeAtomic, and probably
> exception-catch the writeAtomic call.

For the blocklist, we've always erred on the side of using the blocklist data as eagerly as possible. I think we should do the same here.

If the write does fail, it might be a good idea to schedule a re-try (for writing to disk) for X minutes later. This is one of those things where we should try to go that extra mile to try ensure the data is available. Happy for that to be a followup bug.

Comment 9

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8405396 [details] [diff] [review]
Remove main-thread IO from background update of {profile}\blocklist.xml, v3

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

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +589,5 @@
> +
> +    let path = OS.Path.join(OS.Constants.Path.profileDir, FILE_BLOCKLIST);
> +    let encoder = new TextEncoder();
> +    let array = encoder.encode(request.responseText);
> +    yield OS.File.writeAtomic(path, array, {tmpPath: path + ".tmp"});

Based on Blair's input, let's move these four lines to after the this.blocklistUpdated(...) call below, so that the internal data structures are updated whether or not the OS.File.writeAtomic() succeeds.

Also, you can pass the string (request.responseText) directly to writeAtomic(); though undocumented on MDN (yet), it will automatically UTF-8 encode strings.

Also, wrap a try/catch & logger.error(...) around this block of code.

Comment 10

[Roberto Agostino Vitillo (:rvitillo)]

Attachment: Remove main-thread IO from background update of {profile}\blocklist.xml, v4

Comment 11

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8406813 [details] [diff] [review]
Remove main-thread IO from background update of {profile}\blocklist.xml, v4

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

This patch will have a slight conflict with mine for bug 900954 (we change adjacent lines), so you can either land this early or leave it for me to merge the conflict and land them both together after I get r+.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/5cf9041418fa

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5cf9041418fa