Bug 1011158 - Tighten the lock around OS.File |kill| operation. r=froydnj, a=lmandel
authorDavid Rajchenbach-Teller <dteller@mozilla.com>
Mon, 09 Jun 2014 06:25:00 -0400
changeset 207018 4081497dc7c5ff8f62a3ee487ca4ebd37f81ab3b
parent 207017 8a433fd3236d48d6555afd351642fc5dc5f16508
child 207019 95f471af5582e7a2fc9c5ebadfdbc2a6584f09d1
push id3741
push userasasaki@mozilla.com
push dateMon, 21 Jul 2014 20:25:18 +0000
treeherdermozilla-beta@4d6f46f5af68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj, lmandel
bugs1011158
milestone32.0a2
Bug 1011158 - Tighten the lock around OS.File |kill| operation. r=froydnj, a=lmandel
toolkit/components/osfile/modules/osfile_async_front.jsm
toolkit/components/osfile/tests/xpcshell/test_reset.js
--- a/toolkit/components/osfile/modules/osfile_async_front.jsm
+++ b/toolkit/components/osfile/modules/osfile_async_front.jsm
@@ -187,23 +187,31 @@ let Scheduler = {
 
   /**
    * |true| once shutdown has begun i.e. we should reject any
    * message, including resets.
    */
   shutdown: false,
 
   /**
-   * A promise resolved once all operations are complete.
+   * A promise resolved once all currently pending operations are complete.
    *
    * This promise is never rejected and the result is always undefined.
    */
   queue: Promise.resolve(),
 
   /**
+   * A promise resolved once all currently pending `kill` operations
+   * are complete.
+   *
+   * This promise is never rejected and the result is always undefined.
+   */
+  _killQueue: Promise.resolve(),
+
+  /**
    * Miscellaneous debugging information
    */
   Debugging: {
     /**
      * The latest message sent and still waiting for a reply.
      */
     latestSent: undefined,
 
@@ -281,17 +289,24 @@ let Scheduler = {
    * @param {*} options
    * - {boolean} shutdown If |true|, reject any further request. Otherwise,
    *   further requests will resurrect the worker.
    * - {boolean} reset If |true|, instruct the worker to shutdown if this
    *   would not cause leaks. Otherwise, assume that the worker will be shutdown
    *   through some other mean.
    */
   kill: function({shutdown, reset}) {
-    return Task.spawn(function*() {
+    // Grab the kill queue to make sure that we
+    // cannot be interrupted by another call to `kill`.
+    let killQueue = this._killQueue;
+    return this._killQueue = Task.spawn(function*() {
+
+      yield killQueue;
+      // From this point, and until the end of the Task, we are the
+      // only call to `kill`, regardless of any `yield`.
 
       yield this.queue;
 
       // Enter critical section: no yield in this block
       // (we want to make sure that we remain the only
       // request in the queue).
 
       if (!this.launched || this.shutdown || !this._worker) {
--- a/toolkit/components/osfile/tests/xpcshell/test_reset.js
+++ b/toolkit/components/osfile/tests/xpcshell/test_reset.js
@@ -1,75 +1,90 @@
-Components.utils.import("resource://gre/modules/Services.jsm", this);
-Components.utils.import("resource://gre/modules/Promise.jsm", this);
-Components.utils.import("resource://gre/modules/Task.jsm", this);
-Components.utils.import("resource://gre/modules/osfile.jsm", this);
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 let Path = OS.Constants.Path;
 
-add_task(function init() {
+add_task(function* init() {
   do_get_profile();
 });
 
-add_task(function reset_before_launching() {
+add_task(function* reset_before_launching() {
   do_print("Reset without launching OS.File, it shouldn't break");
   yield OS.File.resetWorker();
 });
 
-add_task(function transparent_reset() {
+add_task(function* transparent_reset() {
   for (let i = 1; i < 3; ++i) {
     do_print("Do stome stuff before and after " + i + " reset(s), " +
              "it shouldn't break");
     let CONTENT = "some content " + i;
     let path = OS.Path.join(Path.profileDir, "tmp");
     yield OS.File.writeAtomic(path, CONTENT);
     for (let j = 0; j < i; ++j) {
       yield OS.File.resetWorker();
     }
     let data = yield OS.File.read(path);
     let string = (new TextDecoder()).decode(data);
     do_check_eq(string, CONTENT);
   }
 });
 
-add_task(function file_open_cannot_reset() {
+add_task(function* file_open_cannot_reset() {
   let TEST_FILE = OS.Path.join(Path.profileDir, "tmp-" + Math.random());
   do_print("Leaking file descriptor " + TEST_FILE + ", we shouldn't be able to reset");
   let openedFile = yield OS.File.open(TEST_FILE, { create: true} );
   let thrown = false;
   try {
     yield OS.File.resetWorker();
   } catch (ex if ex.message.indexOf(OS.Path.basename(TEST_FILE)) != -1 ) {
     thrown = true;
   }
   do_check_true(thrown);
 
   do_print("Closing the file, we should now be able to reset");
   yield openedFile.close();
   yield OS.File.resetWorker();
 });
 
-add_task(function dir_open_cannot_reset() {
+add_task(function* dir_open_cannot_reset() {
   let TEST_DIR = yield OS.File.getCurrentDirectory();
   do_print("Leaking directory " + TEST_DIR + ", we shouldn't be able to reset");
   let iterator = new OS.File.DirectoryIterator(TEST_DIR);
   let thrown = false;
   try {
     yield OS.File.resetWorker();
   } catch (ex if ex.message.indexOf(OS.Path.basename(TEST_DIR)) != -1 ) {
     thrown = true;
   }
   do_check_true(thrown);
 
   do_print("Closing the directory, we should now be able to reset");
   yield iterator.close();
   yield OS.File.resetWorker();
 });
 
-add_task(function finish_with_a_reset() {
+add_task(function* race_against_itself() {
+  do_print("Attempt to get resetWorker() to race against itself");
+  // Arbitrary operation, just to wake up the worker
+  try {
+    yield OS.File.read("/foo");
+  } catch (ex) {
+  }
+
+  let all = [];
+  for (let i = 0; i < 100; ++i) {
+    all.push(OS.File.resetWorker());
+  }
+
+  yield Promise.all(all);
+});
+
+
+add_task(function* finish_with_a_reset() {
   do_print("Reset without waiting for the result");
   // Arbitrary operation, just to wake up the worker
   try {
     yield OS.File.read("/foo");
   } catch (ex) {
   }
   // Now reset
   /*don't yield*/ OS.File.resetWorker();