838889 - don't use file.exists() when not necessary (toolkit)

Status: NEW

(Toolkit :: General, defect)

Description

[Marco Castelluccio [:marco]]

Attachment: Part 1

There's a lot of code under toolkit that could use OS.File. I'll file separate bugs for the components that I'll work on.
I'll upload here some simple patches that don't need a separate bug.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Use OS.File in viewSourceUtils.js

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Use OS.File in viewSourceUtils.js v2

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Use OS.File in Telemetry

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Don't use exists() in toolkit/components/search

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Don't use exists() in toolkit/components/search v2

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Part 1

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Use OS.File in about:crashes

Comment 8

[Marco Castelluccio [:marco]]

Attachment: Use OS.File in about:crashes v2

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 711067 [details] [diff] [review]
Use OS.File in viewSourceUtils.js v2

Drew offered to help review these, since I won't be able to get to them for a bit.

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 711067 [details] [diff] [review]
Use OS.File in viewSourceUtils.js v2

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

::: toolkit/components/viewsource/content/viewSourceUtils.js
@@ +257,5 @@
>      },
>  
>      onContentLoaded: function() {
> +      let self = this;
> +      Task.spawn(function() {

What's the point of doing everything in this task?  Could you explain to me what's happening here?  I don't know task.js or OS.File, so sorry if it's obvious.

@@ +273,2 @@
>  
> +            let array = new TextEncoder(self.data.doc.charsetSet).encode(webNavigation.document.body.textContent);

It looks like charsetSet should be characterSet.

Comment 11

[Marco Castelluccio [:marco]]

(In reply to Drew Willcoxon :adw from comment #10)
> What's the point of doing everything in this task?  Could you explain to me
> what's happening here?  I don't know task.js or OS.File, so sorry if it's
> obvious.

It just improves readability, because it makes asynchronous operations look like synchronous ones. And it allows to do just minimal modifications to the existing synchronous code.
There's some documentation here (http://dxr.mozilla.org/mozilla-central/toolkit/content/Task.jsm.html) and here (http://taskjs.org/).

Comment 12

[Drew Willcoxon :adw]

What do you mean by asynchronous?  Is the function passed to spawn called off the main thread, or is it just called on the main thread at some later point?

Comment 13

[Marco Castelluccio [:marco]]

(In reply to Drew Willcoxon :adw from comment #12)
> What do you mean by asynchronous?  Is the function passed to spawn called
> off the main thread, or is it just called on the main thread at some later
> point?

It's called on the main thread and Task.spawn returns a promise.
The OS.File functions, though, are executed off the main thread.

Comment 14

[Drew Willcoxon :adw]

Why do you need the task then?

Comment 15

[Marco Castelluccio [:marco]]

(In reply to Drew Willcoxon :adw from comment #14)
> Why do you need the task then?

It isn't strictly needed, it just improves readability and simplifies the code.

Comment 16

[Drew Willcoxon :adw]

So what's the difference if you took everything you're doing inside the task function, moved it outside the task function, and didn't spawn a task at all?

Comment 17

[:Gavin Sharp [email: [e-mail]]]

The part after the "yield" would need to be put into a callback, which would make it a bit harder to follow.

Comment 18

[Nathan Froyd [:froydnj]]

Comment on attachment 711080 [details] [diff] [review]
Use OS.File in Telemetry

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

This looks pretty good, but there are a few things that need to be fixed.

Also, what's this bug about, exactly?  The summary says that we're not supposed to be using file.exists(), but this code--or at least the code you're replacing--doesn't use file.exists().  If this bug is instead about wholesale conversion to OS.File APIs, then there are a number of other places in TelemetryPing that need to be updated as well.  Please clarify what you're trying to accomplish here.

::: toolkit/components/telemetry/TelemetryPing.js
@@ +800,5 @@
>        }
>        success = true;
>      } catch (e) {
> +      // An error parsing the contents.
> +      OS.File.remove(file.path);

There is no file variable in scope anymore.

@@ +815,5 @@
> +    } catch (e) {
> +      // An error reading the file.
> +      stream.close();           // close is idempotent.
> +      OS.File.remove(file.path);
> +      Telemetry.getHistogramById("READ_SAVED_PING_SUCCESS").add(false);

I'm not excited about how we have this duplicated recording in READ_SAVED_PING_SUCCESS.  Can you refactor loadHistograms/addToPendingPings differently so that we can still record success/failure in one place?

@@ +837,2 @@
>      } else {
> +      let self = this;

Please use (function () { ... }).bind(this) instead.

@@ +911,3 @@
>        let self = this;
> +      let array = new TextEncoder("UTF-8").encode(pingString);
> +      let promise = OS.File.writeAtomic(file.path, array, {tmpPath: file.path + ".tmp"}).then(function onSuccess() {

To ensure the behavior of this function doesn't change, this case also needs to handle the overwrite parameter correctly.

Comment 19

[Drew Willcoxon :adw]

Comment on attachment 711105 [details] [diff] [review]
Don't use exists() in toolkit/components/search v2

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

Is the point of removing exists() calls to avoid stats?

::: toolkit/components/search/nsSearchService.js
@@ +1164,5 @@
> +                         createInstance(Ci.nsIFileInputStream);
> +
> +      fileInStream.init(this._file, MODE_RDONLY, PERMS_FILE, false);
> +    } catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
> +      FAIL("File must exist before calling initFromFile!", Cr.NS_ERROR_UNEXPECTED);

Passing Cr.NS_ERROR_FILE_NOT_FOUND to FAIL would be more precise (like you do elsewhere in the patch).

@@ +2747,5 @@
>      try {
>        stream.init(aFile, MODE_RDONLY, PERMS_FILE, 0);
>        return json.decodeFromStream(stream, stream.available());
> +    } catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
> +      throw Cr.NS_ERROR_FILE_NOT_FOUND;

_readCacheFile shouldn't have to special-case this exception on behalf of a caller that needs it special-cased.

It should just eat all exceptions like it's doing now.  It's OK if it logs file-not-found errors.  Then it should return {} instead of false.  Luckily it has only the one caller.

@@ +3808,5 @@
> +          this._initState = this._InitStates.JSON_LOADING_ATTEMPTED;
> +          // Fall through to the next state
> +        } catch (x) {
> +          LOG("metadata syncInit: could not load JSON file " + x);
> +          this._store = {};

You need a break here.

@@ +3943,5 @@
>        statement.finalize();
>        sqliteDb.close();
> +     } catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
> +       LOG("SRCH_SVC_EMS_migrate search.sqlite does not exist");
> +       return null;

Are you sure that openDatabase throws NS_ERROR_FILE_NOT_FOUND if the file doesn't exist?

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to Nathan Froyd (:froydnj) from comment #18)
> Also, what's this bug about, exactly?  The summary says that we're not
> supposed to be using file.exists(), but this code--or at least the code
> you're replacing--doesn't use file.exists().  If this bug is instead about
> wholesale conversion to OS.File APIs, then there are a number of other
> places in TelemetryPing that need to be updated as well.  Please clarify
> what you're trying to accomplish here.

Yeah, this bug's scope is a bit overly broad now, I think. The telemetry changes are big/complicated enough that they should have their own bug.

Comment 21

[Drew Willcoxon :adw]

Comment on attachment 711110 [details] [diff] [review]
Part 1

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

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +1124,5 @@
>            dbConnection = this._dbBackUpAndRecreate(dbService, dbFile, dbConnection);
>          }
>        }
> +    } catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
> +      dbConnection = this._dbCreate(dbService, dbFile);

Please use a minimal number of trys, and a minimal amount of code within each try.  Here you all you need is something like this, since _dbCreate sets up the connection with the right schema version:

> try {
>   dbConnection = dbService.openDatabase(dbFile);
> }
> catch (e if e.result == Cr.NS_ERROR_FILE_CORRUPTED) {
>   dbConnection = ...
> }
> catch (e if e.result == Cr.NS_ERROR_FILE_NOT_FOUND) {
>   dbConnection = this._dbCreate(...);
> }
> var version = ...
> if (version != ...) { ... }

::: toolkit/components/passwordmgr/storage-mozStorage.js
@@ +1115,5 @@
>              file.append(filename);
>  
> +            let self = this;
> +            OS.File.remove(file.path).then(function onSuccess() {
> +              self.log("Deleting old " + filename + " (" + prefname + ")");

Shouldn't this say "Deleted", since I presume onSuccess is called after the removal has succeeded?

::: toolkit/mozapps/downloads/nsHelperAppDlg.js
@@ +295,5 @@
>        try {
>          // Remove the file so that it's not there when we ensure non-existence later;
>          // this is safe because for the file to exist, the user would have had to
>          // confirm that he wanted the file overwritten.
> +        result.remove(false);

Why did you replace other nsIFile.remove calls with OS.File.remove but not this one?

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +1580,1 @@
>          dbfile.remove(false);

Same question here.

@@ +1710,5 @@
> +          aCallback();
> +      }, function onError(reason) {
> +        if (!(reason instanceof OS.File.error && reason.becauseNoSuchFile)) {
> +          throw reason;
> +        }

You need to call aCallback otherwise.

Comment 22

[Drew Willcoxon :adw]

Comment on attachment 711293 [details] [diff] [review]
Use OS.File in about:crashes v2

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

::: toolkit/crashreporter/content/crashes.js
@@ +111,5 @@
> +    let stat = yield OS.File.stat(submittedDir.path).then(null, function onError() {
> +      exists = false;
> +    };
> +    if (exists && stat.isDir) {
> +      var entries = submittedDir.directoryEntries;

directoryEntries throws if the file doesn't exist or is not actually a directory, so you don't need to stat.

The directoryEntries documentation says it throws NS_ERROR_FILE_NOT_DIRECTORY in the latter case, but in my testing (on a Mac) I see it throws NS_ERROR_FILE_DESTINATION_NOT_DIR.  I also see it throws NS_ERROR_FILE_TARGET_DOES_NOT_EXIST when the file doesn't exist.  (Both errors originate from http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFile.h#52 as far as I can tell.)

It sucks that these exceptions are different from what the documentation says and that they probably depend on each platform's nsIFile implementation.  I think the simplest correct thing to do is to try-catch all exceptions for the single line

  var entries = file.directoryEntries;

If any exception is thrown, assume the file doesn't exist or isn't a directory.

@@ +137,5 @@
> +    stat = yield OS.File.stat(pendingDir.path).then(null, function onError() {
> +      exists = false;
> +    };
> +    if (exists && stat.isDir) {
> +      var entries = pendingDir.directoryEntries;

Here too.

Comment 23

[Marco Castelluccio [:marco]]

Attachment: Use OS.File in viewSourceUtils.js v3

Corrected typo.

Comment 24

[Marco Castelluccio [:marco]]

Attachment: Don't use exists() in toolkit/components/search v3

(In reply to Drew Willcoxon :adw from comment #19)
> Is the point of removing exists() calls to avoid stats?

Yes, it is.

> Are you sure that openDatabase throws NS_ERROR_FILE_NOT_FOUND if the file
> doesn't exist?

If the file doesn't exist SQLite will try to create it, so I'm reverting this change.

Comment 25

[Drew Willcoxon :adw]

Comment on attachment 712000 [details] [diff] [review]
Use OS.File in viewSourceUtils.js v3

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

Normally I'd say that the patch should account for the additional asynchronicity introduced by the task and the writeAtomic.  With this patch, if you were to call gViewSourceUtils.viewSource twice in a row with an external editor enabled, the state of gViewSourceUtils.viewSourceProgressListener would be overwritten by the second call before the spawned task in onContentLoaded had a chance to run in response to the first call.

But the openInExternalEditor path is already broken, because there's already asynchronicity due to the listener by itself, and the path ignores it.  So the patch doesn't break anything like it's not already broken.

But, there's another problem.  If the point is to move potentially long-running work (like I/O) off the main thread, then ideally the text encoding would be done off the main thread too since it's O(length of text), and this patch doesn't do that.  Fortunately it should be straightforward with NetUtil.asyncCopy and nsIScriptableUnicodeConverter.convertToInputStream.

Comment 26

[Drew Willcoxon :adw]

(In reply to Marco Castelluccio [:marco] from comment #24)
> If the file doesn't exist SQLite will try to create it, so I'm reverting
> this change.

Now I'm worried about the other sites where you're catching NS_ERROR_FILE_NOT_FOUND.  Did you check those too?

Comment 27

[Marco Castelluccio [:marco]]

Attachment: Part 1 v2

(In reply to Drew Willcoxon :adw from comment #21)
> Please use a minimal number of trys, and a minimal amount of code within
> each try.

I've reverted that change, because openDatabase doesn't throw that exception.

> ::: toolkit/mozapps/downloads/nsHelperAppDlg.js
> @@ +295,5 @@
> >        try {
> >          // Remove the file so that it's not there when we ensure non-existence later;
> >          // this is safe because for the file to exist, the user would have had to
> >          // confirm that he wanted the file overwritten.
> > +        result.remove(false);
> 
> Why did you replace other nsIFile.remove calls with OS.File.remove but not
> this one?

In this bug I'll do just the simplest fixes. This case requires more changes, because the file should be removed before calling validateLeafName and the promptForSaveToFile function returns the validateLeafName return value. So if the removal is async, also validateLeafName should be async and so also promptForSaveToFile. So I'd need to fix all its callers.

> ::: toolkit/mozapps/extensions/AddonRepository.jsm
> @@ +1580,1 @@
> >          dbfile.remove(false);
> 
> Same question here.

Here because the function is synchronous.

Should I use lazy getters for Task.jsm and osfile.jsm?

Comment 28

[Marco Castelluccio [:marco]]

Attachment: Don't use exists() in toolkit/components/search v3

Comment 29

[Drew Willcoxon :adw]

Comment on attachment 712039 [details] [diff] [review]
Part 1 v2

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

I'm worried about introducing all this concurrency everywhere without preserving the serialization guarantee that comes with the existing on-main-thread I/O.  It's generally just not that easy.

Like, in this patch it's often the case that the last thing a function does is an off-main-thread OS.File.remove, so when control returns to the caller (and its caller and so on), the file may or may not be removed.  I don't see any place where this becomes an obvious problem, but it's breaking what's been up to this point an implicit guarantee, and concurrency bugs arise from complex, subtle interactions that are hard to foresee.

I'm not really comfortable r+'ing if serialization isn't preserved, in whatever manner, which is probably a much bigger task.  Maybe Gavin will disagree.

Comment 30

[David Teller [:Yoric] - still alive but not very active]

(In reply to Drew Willcoxon :adw from comment #25)
> But, there's another problem.  If the point is to move potentially
> long-running work (like I/O) off the main thread, then ideally the text
> encoding would be done off the main thread too since it's O(length of text),
> and this patch doesn't do that.  Fortunately it should be straightforward
> with NetUtil.asyncCopy and
> nsIScriptableUnicodeConverter.convertToInputStream.

We will certainly add the feature to OS.File at some point. In the meantime, you can either use |nsIScriptableUnicodeConverter| or loop |TextEncoder| with option |stream: true| on chunks of the text.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 712042 [details] [diff] [review]
Don't use exists() in toolkit/components/search v3

>diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js

>   _initFromFile: function SRCH_ENG_initFromFile() {

>       FAIL("File must exist before calling initFromFile!", Cr.NS_ERROR_UNEXPECTED);

>   _remove: function SRCH_ENG_remove() {

>-    if (!this._file || !this._file.exists())
>+    if (!this._file)
>       FAIL("Can't remove engine: file doesn't exist!", Cr.NS_ERROR_FILE_NOT_FOUND);

It would be nice to adjust these error messages now that the two checks are split out (these are now just null checks for the argument rather than "file doesn't exist").

Comment 32

[Drew Willcoxon :adw]

Comment on attachment 712039 [details] [diff] [review]
Part 1 v2

I talked with Gavin about the "Part 1" patch, the one with all the OS.File.remove calls.  He adds that it would be better to split that patch into separate bugs.  For modules that manage some kind of storage, like passwordmgr/storage-mozStorage.js, bugs should focus on moving those modules' I/O off the main thread in a holistic manner rather than replacing remove calls here and there.

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Marco, what's the status of this bug? If you're not available to continue work on it, shall I try to find someone else to help?

Comment 34

[Marco Castelluccio [:marco]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #33)
> Marco, what's the status of this bug? If you're not available to continue
> work on it, shall I try to find someone else to help?

Yes, I'm busy at the moment. I'll re-take the bug if I find some time.