Fixes race in Shell

Fixes race in Shell

Generally when a shell test runs something like the following happens:

1. ShellTest (actually BackgroundShell) connects to the test. This
   connection supplies a bound ShellClientRequest.
2. Test code continues on and may attempt to connect to the test as
   well.

Both of these requests may end up going to the ShellResolver. If that
happens, there is no guarantee the ShellResolver will resolve them in
order. If 2 happens first, then we attempt to launch the whole unit
test again, which is completely wrong, and ends up wedging the test.

The fix is twofold:
. Cache ShellResolver in shell. This is necessary as if we use
  separate pipes there is no guaranteed ordering.
. Make Reader use have everything execute in sequence.

BUG=none
TEST=none
R=ben@chromium.org

Committed: https://crrev.com/ba39333a41b90207da717dde0e5178541d10fe33
Cr-Commit-Position: refs/heads/master@{#390136}

[Ben Goodger (Google), 2016-04-26 21:46:26]

lgtm

yowza

[sky, 2016-04-26 21:49:43]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-04-26 21:50:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1921183003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1921183003/1

[commit-bot: I haz the power, 2016-04-26 22:00:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-26 22:00:31]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[sky, 2016-04-26 22:59:18]

Turns out content passes in a different type. I changed the code to take the two
possible taskrunner implements. Ugh!

[sky, 2016-04-26 22:59:26]

Take another look?

[Ben Goodger (Google), 2016-04-27 00:15:38]

ok, lgtm

[sky, 2016-04-27 16:37:12]

Ok, looks like this passes now. We were leaking Shell::Instances that was
DCHECKing. Take another look?

[Ben Goodger (Google), 2016-04-27 17:17:01]

lgtm

[sky, 2016-04-27 17:19:08]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2016-04-27 17:19:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1921183003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1921183003/100001

[commit-bot: I haz the power, 2016-04-27 17:25:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 17:25:27]

Failed to apply patch for services/catalog/reader.cc:
While running git apply --index -3 -p1;
  error: patch failed: services/catalog/reader.cc:172
  Falling back to three-way merge...
  Applied patch to 'services/catalog/reader.cc' with conflicts.
  U services/catalog/reader.cc

Patch:       services/catalog/reader.cc
Index: services/catalog/reader.cc
diff --git a/services/catalog/reader.cc b/services/catalog/reader.cc
index
e97857551cf687108f0992e07da1a1a0bf0dea9e..fcce66e584c269766ef1ca33c8e1ba24dba5dabd
100644
--- a/services/catalog/reader.cc
+++ b/services/catalog/reader.cc
@@ -140,13 +140,23 @@ void DoNothing(shell::mojom::ResolveResultPtr) {}
 
 }  // namespace
 
-Reader::Reader(base::TaskRunner* file_task_runner,
+// A sequenced task runner is used to guarantee requests are serviced in the
+// order requested. To do otherwise means we may run callbacks in an
+// unpredictable order, leading to flake.
+Reader::Reader(base::SequencedWorkerPool* worker_pool,
                ManifestProvider* manifest_provider)
-    : file_task_runner_(file_task_runner),
-      manifest_provider_(manifest_provider),
-      weak_factory_(this) {
-  PathService::Get(base::DIR_MODULE, &system_package_dir_);
+    : Reader(manifest_provider) {
+  file_task_runner_ = worker_pool->GetSequencedTaskRunnerWithShutdownBehavior(
+      base::SequencedWorkerPool::GetSequenceToken(),
+      base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
 }
+
+Reader::Reader(base::SingleThreadTaskRunner* task_runner,
+               ManifestProvider* manifest_provider)
+    : Reader(manifest_provider) {
+  file_task_runner_ = task_runner;
+}
+
 Reader::~Reader() {}
 
 void Reader::Read(const base::FilePath& package_dir,
@@ -172,21 +182,25 @@ void Reader::CreateEntryForName(
     std::unique_ptr<base::Value> manifest_root =
         base::JSONReader::Read(manifest_contents);
     base::PostTaskAndReplyWithResult(
-        file_task_runner_, FROM_HERE,
+        file_task_runner_.get(), FROM_HERE,
         base::Bind(&ProcessManifest, base::Passed(&manifest_root),
                    system_package_dir_),
-        base::Bind(&Reader::OnReadManifest, weak_factory_.GetWeakPtr(),
-                   cache, entry_created_callback));
+        base::Bind(&Reader::OnReadManifest, weak_factory_.GetWeakPtr(), cache,
+                   entry_created_callback));
   } else {
     base::PostTaskAndReplyWithResult(
-        file_task_runner_, FROM_HERE,
+        file_task_runner_.get(), FROM_HERE,
         base::Bind(&ReadManifest, system_package_dir_, mojo_name),
-        base::Bind(&Reader::OnReadManifest, weak_factory_.GetWeakPtr(),
-                   cache, entry_created_callback));
-
+        base::Bind(&Reader::OnReadManifest, weak_factory_.GetWeakPtr(), cache,
+                   entry_created_callback));
   }
 }
 
+Reader::Reader(ManifestProvider* manifest_provider)
+    : manifest_provider_(manifest_provider), weak_factory_(this) {
+  PathService::Get(base::DIR_MODULE, &system_package_dir_);
+}
+
 void Reader::OnReadManifest(
     EntryCache* cache,
     const CreateEntryForNameCallback& entry_created_callback,

[commit-bot: I haz the power, 2016-04-27 17:25:36]

The CQ bit was unchecked by commit-bot@chromium.org

[sky, 2016-04-27 17:31:47]

The CQ bit was checked by sky@chromium.org

[sky, 2016-04-27 17:31:47]

The patchset sent to the CQ was uploaded after l-g-t-m from ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1921183003/#ps120001
(title: "merge 2 trunk")

[commit-bot: I haz the power, 2016-04-27 17:32:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1921183003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1921183003/120001

[commit-bot: I haz the power, 2016-04-27 17:35:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-27 17:35:04]

Try jobs failed on following builders:
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2016-04-27 17:40:07]

The CQ bit was checked by sky@chromium.org

[sky, 2016-04-27 17:40:07]

The patchset sent to the CQ was uploaded after l-g-t-m from ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1921183003/#ps140001
(title: "merge 2 trunk")

[commit-bot: I haz the power, 2016-04-27 17:40:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1921183003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1921183003/140001

[commit-bot: I haz the power, 2016-04-27 18:44:26]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-30 17:11:32]

Patchset 8 (id:??) landed as
https://crrev.com/ba39333a41b90207da717dde0e5178541d10fe33
Cr-Commit-Position: refs/heads/master@{#390136}

[commit-bot: I haz the power, 2016-04-30 17:51:02]

Description was changed from

==========
Fixes race in Shell

Generally when a shell test runs something like the following happens:

1. ShellTest (actually BackgroundShell) connects to the test. This
   connection supplies a bound ShellClientRequest.
2. Test code continues on and may attempt to connect to the test as
   well.

Both of these requests may end up going to the ShellResolver. If that
happens, there is no guarantee the ShellResolver will resolve them in
order. If 2 happens first, then we attempt to launch the whole unit
test again, which is completely wrong, and ends up wedging the test.

The fix is twofold:
. Cache ShellResolver in shell. This is necessary as if we use
  separate pipes there is no guaranteed ordering.
. Make Reader use have everything execute in sequence.

BUG=none
TEST=none
R=ben@chromium.org
==========

to

==========
Fixes race in Shell

Generally when a shell test runs something like the following happens:

1. ShellTest (actually BackgroundShell) connects to the test. This
   connection supplies a bound ShellClientRequest.
2. Test code continues on and may attempt to connect to the test as
   well.

Both of these requests may end up going to the ShellResolver. If that
happens, there is no guarantee the ShellResolver will resolve them in
order. If 2 happens first, then we attempt to launch the whole unit
test again, which is completely wrong, and ends up wedging the test.

The fix is twofold:
. Cache ShellResolver in shell. This is necessary as if we use
  separate pipes there is no guaranteed ordering.
. Make Reader use have everything execute in sequence.

BUG=none
TEST=none
R=ben@chromium.org

Committed: https://crrev.com/ba39333a41b90207da717dde0e5178541d10fe33
Cr-Commit-Position: refs/heads/master@{#390136}
==========