Port https://sites.google.com/a/chromium.org/dev/developers/design-documents/threading/suble-thread…

docs/subtle_threading_bugs.md

Index: docs/subtle_threading_bugs.md
diff --git a/docs/subtle_threading_bugs.md b/docs/subtle_threading_bugs.md
new file mode 100644
index 0000000000000000000000000000000000000000..5ab7cf92765e4fea364718b494359f93f0d0f7dc
--- /dev/null
+++ b/docs/subtle_threading_bugs.md
@@ -0,0 +1,133 @@
+# Subtle Threading Bugs and Patterns to avoid them
+
+[TOC]
+
+## The Problem
+We were using a number of patterns that were problematic:
+
+1. Using `BrowserThread::GetMessageLoop` This isn't safe, since it could return
+   a valid pointer, but since the caller isn't holding on to a lock anymore, the
+   target MessageLoop could be destructed while it's being used.
+   
+1. Caching of MessageLoop pointers in order to use them later for PostTask and friends
+
+   * This was more efficient previously (more on that later) since using
+     `BrowserThread::GetMessageLoop` involved a lock.
+   
+   * But it spread logic about the order of thread destruction all over the
+     code.  Code that moved from the IO thread to the file thread and back, in
+     order to avoid doing disk access on the IO thread, ended up having to do an
+     extra hop through the UI thread on the way back to the IO thread since the
+     file thread outlives the IO thread.  Of course, most code learnt this the
+     hard way, after doing the straight forward IO->file->IO thread hop and
+     updating the code after seeing reliability or user crashes.
+
+   * It made the browser shutdown fragile and hence difficult to update.
+   
+1. File thread hops using RefCountedThreadSafe objects which have non-trivial
+   destructors
+
+   * To reduce jank, frequently an object on the UI or IO thread would execute
+     some code on the file thread, then post the result back to the original
+     thread.  We make this easy using `base::Callback` and
+     `RefCountedThreadSafe`, so this pattern happened often, but it's not always
+     safe: base::Callback holds an extra reference on the object to ensure that
+     it doesn't invoke a method on a deleted object.  But it's quite possible
+     that before the file thread's stack unwinds and it releases the extra
+     reference, that the response task on the original thread executed and
+     released its own additional reference.  The file thread is then left with
+     the remaining reference and the object gets destructed there.  While for
+     most objects this is ok, many have non-trivial destructors, with the worst
+     being ones that register with the UI-thread NotificationService.  Dangling
+     pointers would be left behind and tracking these crashes from ChromeBot or
+     the crash dumps has wasted several days at least for me.
+
+1. Having browser code take different code paths if a thread didn't exist
+
+   * This could be either deceptively harmless (i.e. execute synchronously when
+     it was normally asynchronous), when in fact it makes shutdown slower
+     because disk access is moved to the UI thread.
+ 
+   * It could lead to data loss, if tasks are silently not posted because the
+     code assumes this only happens in unit tests, when it could occur on
+     browser shutdown as well.
+
+## The Solution
+
+* 1+2: Where possible, use `BrowserThread::PostTask`. Everywhere else, use
+  `scoped_refptr<MessageLoopProxy>`.
+
+  `BrowserThread::PostTask` and friends (i.e. `PostDelayedTask`, `DeleteSoon`,
+  `ReleaseSoon`) are safe and efficient: no locks are grabbed if the target
+  thread is known to outlive the current thread.  The four static methods have
+  the same signature as the ones from `MessageLoop`, with the addition of the
+  first parameter to indicate the target thread:
+
+    `BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, task);`
+
+  Similarly, `MessageLoopProxy` has (non-static) methods with the same signature
+  as the `MessageLoop` counterparts, but is safe for caching a [reference
+  counting] pointer to. You can obtain a `MessageLoopProxy` via
+  `Thread::message_loop_proxy()`, `BrowserThread::GetMessageLoopProxy()`, or
+  `MessageLoopProxy::CreateForCurrentThread()`.
+
+* 3: If you want to execute a method on another thread and jump back to the
+  original thread, use `PostTaskAndReply()`:
+
+    BrowserThread::PostTaskAndReply(BrowserThread::FILE, FROM_HERE,
+                                    base::Bind(&Foo::DoStuffOnFileThread, this),
+                                    base::Bind(&Foo::StuffDone, this));
+
+  `PostTaskAndReply()` will make sure that both tasks are destroyed on the
+  thread they were created on, so if they hold the last reference to the object,
+  it will be destroyed on the originating thread. You can also use
+  `PostTaskAndReplyWithResult()` to return a value from the first task and pass
+  it into the second task.
+  
+  Alternatively, if your object must be destructed on a specific thread, you can
+  use a trait from BrowserThread:
+
+    class Foo : public base::RefCountedThreadSafe<Foo, BrowserThread::DeleteOnIOThread>
+
+* 4: I've removed all the special casing and always made the objects in the
+  browser code behave in one way.  If you're writing a unit test and need to use
+  an object that goes to a file thread (where before it would proceed
+  synchronously), you just need:
+
+    BrowserThread file_thread(BrowserThread::FILE, MessageLoop::current());
+    foo->StartAsyncTaskOnFileThread();
+    MessageLoop::current()->RunAllPending();
+
+  There are plenty of examples now in the tests.
+
+## Gotchas
+Even when using `BrowseThread` or `MessageLoopProxy`, you will still likely have
+messages lost (usually resulting in memory leaks) when the target thread is in
+the process of shutting down: the benefit over `MessageLoop` is primarily one of
+avoiding crashing in unpredictable ways. (See this thread for debate.)
+
+`BrowseThread` and `MessageLoopProxy::PostTask` will silently delete a task if
+the thread doesn't exist.  This is done to avoid having all the code that uses
+it have special cases if the target thread exists or not, and to make Valgrind
+happy.  As usual, the task for `DeleteSoon/ReleaseSoon` doesn't do anything in
+its destructor, so this won't cause unexpected behavior with them.  But be wary
+of posted `Task` objects which have non-trivial destructors or smart pointers as
+members.  I'm still on the fence about this, since while the latter is
+theoretical now, it could lead to problems in the future.  I might change it so
+that the tasks are not deleted when I'm ready for more Valgrind fun.
+
+If you absolutely must know if a task was posted or not, you can check the
+return value of `PostTask` and friends.  But note that even if the task was
+posted successfully, there's no guarantee that it will run because the target
+thread could already have a `QuitTask` queued up, or be in the early stages of
+quitting.
+
+`g_browser->io_thread()` and `file_thread()` are still around (the former for
+IPC code, and the latter for Linux proxy code which is in net and so can't use
+`BrowserThread`).  Don't use them unless absolutely necessary.
+
+
+### More information
+
+* https://bugs.chromium.org/p/chromium/issues/detail?id=25354
+