867776 - Main-thread I/O in URL Classifier

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Vladan Djeric (:vladan)]

URL classifier's PROT_UrlCryptoKeyManager.prototype.serializeKey opens/writes/closes "urlclassifierkey3.txt" stream on main thread

Profile: http://people.mozilla.com/~bgirard/cleopatra/#report=c934a0182d38178331a2bbce35a62f6a190aad74&search=urlclassifierlib.js

Is it possible to move these operations off the main thread, e.g. using OS.File?

Comment 1

[Doug Turner (:dougt)]

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/url-crypto-key-manager.js#266

dcamp, is this your file?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Doug Turner (:dougt) from comment #1)
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-
> classifier/content/url-crypto-key-manager.js#266
> 
> dcamp, is this your file?

Doug, Welcome to 2013. DCamp is no longer in URL Classifier. CC'ing GCP.

Comment 3

[Doug Turner (:dougt)]

gcp, mfinkle tells me it isn't 2013 and IO shouldn't run on the main thread.  can you halp?

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Sounds like a great idea, maybe someone familiar with that file, or JavaScript and off-main-thread IO can take a look at it. 

Bug 779317 is somewhat relevant here, from some quick digging the setKeyUrl causes the actual keyServiceThingManager to be initialized, which is the one doing main-thread IO.

I think you can make all of that run asynchronously and the call SafeBrowsing.controlUpdateChecking at the end. This is a wild guess looking at the XXX'es in SafeBrowsing.jsm near the readPrefs() function.

>can you halp?

Yes, I'll r+ any patches that don't completely seem to break things!

Comment 5

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

I'm willing to review/mentor/feedback the "off-main-thread IO in JavaScript" part.

Comment 6

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

Actually, this looks like ~5 lines of code with OS.File, so that's a good first bug/mentored bug.

Comment 7

[Sankha Narayan Guria [:sankha]]

Yoric, can I pick this up?

Comment 8

[Sankha Narayan Guria [:sankha]]

Attachment: WIP Patch with OS.File

Wrote this patch, can you give your feedback?

Comment 9

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

Comment on attachment 745737 [details] [diff] [review]
WIP Patch with OS.File

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

Looks good to me, with a few changes.

::: toolkit/components/url-classifier/content/url-crypto-key-manager.js
@@ +282,5 @@
>      }
>  
>      var data = (new G_Protocol4Parser()).serialize(map);
>  
> +    Cu.import("resource://gre/modules/osfile.jsm");

You should put this import at toplevel.

@@ +285,5 @@
>  
> +    Cu.import("resource://gre/modules/osfile.jsm");
> +    let encoder = new TextEncoder();
> +    let array = encoder.encode(data);
> +    let promise = OS.File.writeAtomic(keyfile, array, {tmpPath: keyfile + ".tmp"});

I'm not completely sure about the type of |data|, but if its a typed array, you should be able to pass it directly as second argument to |writeAtomic| without having to mess with |encoder|.

@@ +286,5 @@
> +    Cu.import("resource://gre/modules/osfile.jsm");
> +    let encoder = new TextEncoder();
> +    let array = encoder.encode(data);
> +    let promise = OS.File.writeAtomic(keyfile, array, {tmpPath: keyfile + ".tmp"});
> +    promise = promise.then(function onSuccess(value) {

You are not using |value|. Don't declare it.

@@ +288,5 @@
> +    let array = encoder.encode(data);
> +    let promise = OS.File.writeAtomic(keyfile, array, {tmpPath: keyfile + ".tmp"});
> +    promise = promise.then(function onSuccess(value) {
> +      return true;
> +    }, function onFailure(value) {

You probably mean |e| instead of |value|.

@@ +289,5 @@
> +    let promise = OS.File.writeAtomic(keyfile, array, {tmpPath: keyfile + ".tmp"});
> +    promise = promise.then(function onSuccess(value) {
> +      return true;
> +    }, function onFailure(value) {
> +      G_Error(this, "Failed to serialize new key: " + e);

Since you use |this|, you should bind |onFailure| to |this| first.

@@ +291,5 @@
> +      return true;
> +    }, function onFailure(value) {
> +      G_Error(this, "Failed to serialize new key: " + e);
> +      return false;
> +    });

You should return |promise|. Also, please update the documentation of |serializeKey_| to mention that it returns the promise of a boolean.

Comment 10

[Sankha Narayan Guria [:sankha]]

Attachment: patch with comments addressed

Comment 11

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

Comment on attachment 747984 [details] [diff] [review]
patch with comments addressed

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

::: toolkit/components/url-classifier/content/url-crypto-key-manager.js
@@ +293,5 @@
> +      return promise;
> +    };
> +    failureHandler.bind(this);
> +    promise = promise.then(function onSuccess() {
> +      return promise;

No, here return your boolean.

@@ +294,5 @@
> +    };
> +    failureHandler.bind(this);
> +    promise = promise.then(function onSuccess() {
> +      return promise;
> +    }, failureHandler);

You don't need to create an explicit function for failureHandler.

@@ +294,5 @@
> +    };
> +    failureHandler.bind(this);
> +    promise = promise.then(function onSuccess() {
> +      return promise;
> +    }, failureHandler);

...here, return your promise.

Comment 12

[Sankha Narayan Guria [:sankha]]

Attachment: Patch returning promise

Comment 13

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

Comment on attachment 747990 [details] [diff] [review]
Patch returning promise

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

Fine with me if it passes tests.

Comment 14

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

Comment on attachment 747990 [details] [diff] [review]
Patch returning promise

>diff --git a/toolkit/components/url-classifier/content/url-crypto-key-manager.js b/toolkit/components/url-classifier/content/url-crypto-key-manager.js

> PROT_UrlCryptoKeyManager.prototype.serializeKey_ = function() {

>     if (!this.clientKey_ || !this.wrappedKey_) {
>       keyfile.remove(true);

We probably want to convert this to use OS.File as well.

>+    let promise = OS.File.writeAtomic(keyfile, array, {tmpPath: keyfile + ".tmp"});

Shouldn't this be keyfile.path? Is there existing test coverage for this code?

It looks to me like the PROT_ListManager.prototype.newKey_ is eventually called synchronously after serializeKey_, via PROT_UrlCryptoKeyManager.prototype.replaceKey_. Are you sure nothing depends on the I/O here being synchronous?

(This code looks like a horrible mess of abstraction that is mostly unnecessary. I wonder whether there are bugs on cleaning it up?)

Comment 15

[Sankha Narayan Guria [:sankha]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #14)

> > PROT_UrlCryptoKeyManager.prototype.serializeKey_ = function() {
> 
> >     if (!this.clientKey_ || !this.wrappedKey_) {
> >       keyfile.remove(true);
> 
> We probably want to convert this to use OS.File as well.

Sure I'll do that.

> >+    let promise = OS.File.writeAtomic(keyfile, array, {tmpPath: keyfile + ".tmp"});
> 
> Shouldn't this be keyfile.path? Is there existing test coverage for this
> code?

Should I change the current implementation that uses nsILocalFile to FileUtils.jsm? The code would be simpler, I think.

> It looks to me like the PROT_ListManager.prototype.newKey_ is eventually
> called synchronously after serializeKey_, via
> PROT_UrlCryptoKeyManager.prototype.replaceKey_. Are you sure nothing depends
> on the I/O here being synchronous?

No idea bout this. Maybe someone who knows about this can tell.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

>Is there existing test coverage for this code?

Generally speaking, there is zero test coverage for any part of UrlClassifier that depends on interacting with the SafeBrowsing servers (because we don't have an independent implementation). Individual parts may have tests that may (but probably won't) flag when you completely break them. 

>Maybe someone who knows about this can tell.

The people who know have left a long time ago and work on a competing product now. Your best bet will be to investigate the code yourself.

Comment 17

[Sankha Narayan Guria [:sankha]]

Attachment: refactored patch

In this patch, I have refactored the serializeKey_ method to do all async IO. All the existing tests are passing with this patch.

Comment 18

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

Comment on attachment 756900 [details] [diff] [review]
refactored patch

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

This should handle serialization. We should probably open a followup bug to handle loading, which might be a little more complicated.

::: toolkit/components/url-classifier/content/url-crypto-key-manager.js
@@ +272,5 @@
>    var map = {};
>    map[this.CLIENT_KEY_NAME] = this.clientKey_;
>    map[this.WRAPPED_KEY_NAME] = this.wrappedKey_;
>    
>    try {  

Could you take the opportunity to remove the whitespaces at the end of the two previous lines?

@@ +280,5 @@
>      if (!this.clientKey_ || !this.wrappedKey_) {
> +      let promise = OS.File.remove(keyfile);
> +      promise.then(function completeRemoval() {
> +        return;
> +      }.bind(this));

No, that's not how it works.
If you just want to return after having launched (asynchronous removal), do

  OS.File.remove(keyfile);
  return; // Return without waiting for removal to be complete.

What you are doing here will not stop execution of |serializeKey_|, so it will break the intended semantics.

@@ +287,5 @@
>      var data = (new G_Protocol4Parser()).serialize(map);
>  
> +    let encoder = new TextEncoder();
> +    let array = encoder.encode(data);
> +    let promise = OS.File.writeAtomic(keyfile, array, {tmpPath: keyfile + ".tmp"});

From the source code of the original version, I deduce that we do not need extreme safety for this |writeAtomic|. So, please add option |flush: false|, to speed things up.

@@ +290,5 @@
> +    let array = encoder.encode(data);
> +    let promise = OS.File.writeAtomic(keyfile, array, {tmpPath: keyfile + ".tmp"});
> +    promise = promise.then(function onSuccess() {
> +      return true;
> +    }.bind(this), function onFailure(e) {

You don't need the |bind| above.

@@ +294,5 @@
> +    }.bind(this), function onFailure(e) {
> +      G_Error(this, "Failed to serialize new key: " + e);
> +      return false;
> +    }.bind(this));
> +    return promise;

Since you aren't doing anything else with the promise, you can |return promise.then(...)| immediately above, instead of this |promise = promise.then(...)|.

Comment 19

[Sankha Narayan Guria [:sankha]]

Attachment: final patch

I have addressed your comments in this patch.

Talking about the follow-up bug to handle the loading asynchronously, is the bug filed or should I file one?

Comment 20

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

Filed as bug 879724.

Comment 21

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

Comment on attachment 758511 [details] [diff] [review]
final patch

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

This looks almost good. However, it seems that this code path is not covered by any test.
Before proceeding, you should probably find a way to add a test for |serializeKey_|.

::: toolkit/components/url-classifier/content/url-crypto-key-manager.js
@@ +257,5 @@
> +  promise.then(function fireCallback() {
> +    if (this.onNewKey_) {
> +      this.onNewKey_();
> +    }
> +  });

This won't work, missing |bind|.

@@ +279,3 @@
>  
>      if (!this.clientKey_ || !this.wrappedKey_) {
> +      OS.File.remove(keyfile);

You should document why you can remove the file asynchronously and proceed without waiting for the file to be actually removed.

You might wish to check this with dcamp or ehsan first, to ensure that this is actually true.

Comment 22

[(no longer active)]

I don't know anything about the URL classifier!  :-)

Comment 23

[Sankha Narayan Guria [:sankha]]

Attachment: patch with tests

I added a new xpcshell test, and got rid of the old test code. Apparently that old test function is not called from anywhere.

Comment 24

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #22)
> I don't know anything about the URL classifier!  :-)

You should, you appear in its hg logs :)

Comment 25

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

Comment on attachment 759135 [details] [diff] [review]
patch with tests

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

Mostly good. Let's clean up a few things and then let's get dcamp to review that code.

::: toolkit/components/url-classifier/content/url-crypto-key-manager.js
@@ +257,5 @@
> +  promise.then(function fireCallback() {
> +    if (this.onNewKey_) {
> +      this.onNewKey_();
> +    }
> +  }.bind(this));

Nit: The following would be equivalent but slightly more readable

promise.then(() =>
  if (this.onNewKey_) {
    this.onNewKey_();
  }
);

@@ +277,2 @@
>  
> +    let keyfile = OS.Path.join(OS.Constants.Path.profileDir, this.keyFilename_);

Nit: Could you rename this |keypath|?

@@ +277,4 @@
>  
> +    let keyfile = OS.Path.join(OS.Constants.Path.profileDir, this.keyFilename_);
> +
> +    // if we have an invalid client key or wrapped key, we remove the 

Nit: Trailing whitespace.

@@ +282,2 @@
>      if (!this.clientKey_ || !this.wrappedKey_) {
> +      OS.File.remove(keyfile);

Have you checked with dcamp that it's ok to return without having waited for the file to be effectively removed?

@@ +284,4 @@
>        return;
>      }
>  
>      var data = (new G_Protocol4Parser()).serialize(map);

Nit: Could you take the opportunity to replace this |var| with a |let|?

@@ +294,5 @@
> +      return true;
> +    }, function onFailure(e) {
> +      G_Error(this, "Failed to serialize new key: " + e);
> +      return false;
> +    }.bind(this));

Actually, this would also be improved by using =>.

::: toolkit/components/url-classifier/tests/unit/test_keymanager.js
@@ +1,1 @@
> +function run_test() {    

Could you add the Public Domain header?
Also, nit: trailing whitepace.
Also, given that the stuff is somewhat asynchronous, you will probably want to wrap your test in a call to |add_task|.

function run_test() {
  run_next_test();
}

add_task(function test() {
 // All your code
});

@@ +1,4 @@
> +function run_test() {    
> +    var jslib = Cc["@mozilla.org/url-classifier/jslib;1"]
> +                .getService().wrappedJSObject;
> +    Cu.import("resource://gre/modules/osfile.jsm");

Please don't put the Cu.import between these two statements. Put it either before or after.

@@ +7,5 @@
> +    var kf = "keytest.txt"; // not an actual keyfile
> +    // we should clean up after the test
> +    function removeTestFile(f) {
> +        OS.File.remove(kf).then(function() {
> +            return;

That call to |then| is pointless. What are you attempting to do?

Comment 26

[Sankha Narayan Guria [:sankha]]

Attachment: patch

Comment 27

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

Comment on attachment 760906 [details] [diff] [review]
patch

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

Are you sure that your test is executed? It looks like your patch is syntactically incorrect.
Also, I'd still like dcamp to confirm explicitly that returning without waiting for the OS.File.remove() operation to be complete is correct. Re-f? just in case that escaped him :)

::: toolkit/components/url-classifier/content/url-crypto-key-manager.js
@@ +284,1 @@
>        return;

return OS.File.remove(keypath).then(() => true, () => false);
Otherwise, you're not respecting the @returns documentation above.

@@ +289,5 @@
> +    let encoder = new TextEncoder();
> +    let array = encoder.encode(data);
> +    let promise = OS.File.writeAtomic(keypath, array, { tmpPath: keypath + ".tmp",
> +                                                        flush:   false });
> +    return promise.then(onSuccess: () => return true,

Does that work? That doesn't look syntactically correct to me.
Also, you don't need the |return|.

@@ +293,5 @@
> +    return promise.then(onSuccess: () => return true,
> +                        onFailure: e => {
> +      G_Error(this, "Failed to serialize new key: " + e);
> +      return false;
> +    });

Does that work? I believe that we have a |try| without a |catch|.

::: toolkit/components/url-classifier/tests/unit/test_keymanager.js
@@ +16,5 @@
> +    // we should clean up after the test
> +    function removeTestFile(f) {
> +        OS.File.remove(kf);
> +    };
> +    removeTestFile(kf);

1/ That function |removeTestFile| is useless, you can directly call |OS.File.remove|;
2/ You're not using |f|;
3/ You probably want to wait until the removal is complete before proceeding, as follows:
  yield OS.File.remove(kf);

@@ +18,5 @@
> +        OS.File.remove(kf);
> +    };
> +    removeTestFile(kf);
> +
> +    // CASE: simulate nothing on disk, then get something from server

Nit: Could you replace |// CASE:| comments with a call |do_print| and the same comments? This generally makes it easier to find out where errors come from, in case of problem.

Comment 28

[Sankha Narayan Guria [:sankha]]

Attachment: patch

dcamp: Can you please check resturning a promise from the OS.File.remove operation in serializeKey_, without waiting for it to complete, will not have any side effect?

Yoric: I have made the changes that you have suggested.

Comment 29

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

Comment on attachment 771148 [details] [diff] [review]
patch

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

Looks good to me. Waiting for Dave's answer.

::: toolkit/components/url-classifier/content/url-crypto-key-manager.js
@@ +282,2 @@
>      if (!this.clientKey_ || !this.wrappedKey_) {
> +      return OS.File.remove(keypath).then(() => true, () => false);

Mmmh...
Actually, the original code didn't match its documentation, as it returned undefined. Oh, well, there isn't much you can do.

::: toolkit/components/url-classifier/tests/unit/test_keymanager.js
@@ +5,5 @@
> +Cu.import("resource://gre/modules/osfile.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");
> +
> +function run_test() {
> +    do_test_pending();

You shouldn't need do_test_pending().

@@ +69,5 @@
> +
> +    yield OS.File.remove(kf);
> +});
> +
> +add_task(do_test_finished);

If you remove do_test_pending, I believe that you can remove do_test_finished.

Comment 30

[Dave Camp (:dcamp)]

Comment on attachment 771148 [details] [diff] [review]
patch

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

Honestly, I barely touched this file 4-5 years ago, so your understanding is probably as good as mine :)

But I think this should be fine now that you wait on that promise.

Comment 31

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

Looks good.
You can mark this patch checkin-needed as soon as you are satisfied that it passes all tests. Remind me, do you have try access, sankha93?

Comment 32

[Sankha Narayan Guria [:sankha]]

Attachment: final patch

Made the small changes as suggested by Yoric.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ad4243db7fce

Comment 33

[Sankha Narayan Guria [:sankha]]

Pushed to try, after fixing small mistakes: https://tbpl.mozilla.org/?tree=Try&rev=68cb5b7c7b67

The tests are sometimes passing and sometimes failing. Is it because we are not waiting for OS.File.remove to be completed?

Comment 34

[Marco Castelluccio [:marco]]

I think the problem is you've made onGetKeyResponse, maybeLoadOldKey and dropKey asynchronous, you should make sure that their callers can work anyway.
You're also using them in the test as if they were synchronous.

Comment 35

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

So, what's the status of this bug, sankha93?

Comment 36

[Sankha Narayan Guria [:sankha]]

The tests are passing on my local machine, but sometimes pass/fail on Try, so I am having a hard time debugging.

I will talk to you in details on IRC about this, as soon as I return home from the Summit.

Comment 37

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

Could you post a link to the latest Try?

Comment 38

[Sankha Narayan Guria [:sankha]]

tbpl: https://tbpl.mozilla.org/?tree=Try&rev=02159fa56ccc

Comment 39

[Sankha Narayan Guria [:sankha]]

Attachment: patch with green on try

This patch is green on try. https://tbpl.mozilla.org/?tree=Try&rev=2b0c99f9193b

Marking checkin-needed as per comment 31.

Comment 40

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

That's not really what I meant in comment 31.
Let's take a look.

Comment 41

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

Comment on attachment 817896 [details] [diff] [review]
patch with green on try

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

::: toolkit/components/url-classifier/content/url-crypto-key-manager.js
@@ +299,5 @@
>  /**
>   * Invoked when we've received a protocol4 response to our getkey
>   * request. Try to parse it and set this key as the new one if we can.
>   *
>   *  @param responseText String containing the protocol4 getkey response

Please update the documentation to mention the fact that you return a promise.

@@ +317,2 @@
>    } else {
>      G_Debug(this, "Not a valid response for /newkey");

Please return a promise here, too.

@@ +380,1 @@
>    }

Please make sure that this method always returns a promise. Also, please update the documentation.

Comment 42

[Sankha Narayan Guria [:sankha]]

Attachment: patch v10

Comment 43

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

Comment on attachment 817912 [details] [diff] [review]
patch v10

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

A few errors with promises.

::: toolkit/components/url-classifier/content/url-crypto-key-manager.js
@@ +42,5 @@
> +
> +// A deferred that returns a promise that always resolves to false
> +
> +let falseDeferred = Promise.defer();
> +falseDeferred.resolve(false);

I understand the feeling, but that looks like an unneeded optimization. Just return |Promise.resolve(false);| from your methods.

@@ +267,5 @@
> +      this.onNewKey_();
> +    }
> +    return true;
> +  });
> +  return promise;

No, that's the wrong promise you are returning.
You should return the result of your |promise.then(() ... )|.

@@ +302,4 @@
>      G_Error(this, "Failed to serialize new key: " + e);
>      return false;
> +  });
> +  return promise;

Here, too, you are returning the wrong promise.

@@ +316,1 @@
>   */ 

Nit: Could you take the opportunity to remove that whitespace at the end of the line?

@@ +329,3 @@
>    } else {
>      G_Debug(this, "Not a valid response for /newkey");
> +    return falseDeferred.promise;

Just |return Promise.resolve(false)|

@@ +356,1 @@
>   */ 

Here, too, could you take the opportunity to remove that whitespace?

@@ +373,5 @@
>          oldKey = stream.read(stream.available());
>        } finally {
>          if (stream)
>            stream.close();
>        }

Just to refresh my memory: removing that main thread I/O is planned for bug 879724, is that it?

@@ +377,5 @@
>        }
>      }
>    } catch(e) {
>      G_Debug(this, "Caught " + e + " trying to read keyfile");
> +    return falseDeferred.promise;

As above.

@@ +382,5 @@
>    }
>     
>    if (!oldKey) {
>      G_Debug(this, "Couldn't find old key.");
> +    return falseDeferred.promise;

As above.

@@ +396,1 @@
>    }

You should also return a promise in that case. I *think* it should be Promise.resolve(false).

Comment 44

[Sankha Narayan Guria [:sankha]]

Attachment: patch v11

> Just to refresh my memory: removing that main thread I/O is planned for bug 879724, is that it?

Yes.

Comment 45

[Sankha Narayan Guria [:sankha]]

Attachment: patch v11

fixed alignment

Comment 46

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

Comment on attachment 822956 [details] [diff] [review]
patch v11

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

r=me with the change below

::: toolkit/components/url-classifier/content/url-crypto-key-manager.js
@@ +281,5 @@
>  
> +  // if we have an invalid client key or wrapped key, we remove the
> +  // invalid keyfile from disk
> +  if (!this.clientKey_ || !this.wrappedKey_) {
> +    return OS.File.remove(keypath).then(() => true, () => false);

That sounds suspicious. You return |true| when the previous version returned |undefined|, i.e. almost |false|, so you are changing the semantics of this function. You should rather always return |false|.

Also, in the |onError| callback, you should re-throw the error *unless* this is an |ex| such that |ex.becauseNoSuchFile| is |false|, in which case we cannot remove the file simply because it has been removed already.

Comment 47

[Sankha Narayan Guria [:sankha]]

Attachment: patch v12

Asking for review just to ensure that what I did for OS.File.remove is correct.

Also pushed to Try: https://tbpl.mozilla.org/?tree=Try&rev=2240aca2876b

Comment 48

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

Comment on attachment 825155 [details] [diff] [review]
patch v12

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

Looks good to me, thanks for the fix.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

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

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f66a6b1b0580