Closed Bug 989419 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: Irving, Assigned: rvitillo)

References

(Blocks 1 open bug)

Details

(Keywords: main-thread-io)

Attachments

(1 file, 3 obsolete files)

+++ 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.
Blair, do you think it's worth going ahead with this before you get around to rewriting the service (bug bug 782261)?
Flags: needinfo?(bmcbride)
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.
Flags: needinfo?(bmcbride)
Assignee: nobody → rvitillo
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.
Attachment #8402732 - Flags: review?(irving) → feedback+
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.
Attachment #8403257 - Flags: review?(irving) → review+
Flags: needinfo?(bmcbride)
(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.
Flags: needinfo?(bmcbride)
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.
Attachment #8405396 - Flags: feedback+
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+.
Attachment #8406813 - Flags: review?(irving) → review+
https://hg.mozilla.org/mozilla-central/rev/5cf9041418fa
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.