Fix use-after-free triggered from mojo::SyncEventWatcher DTOR

This is something I've found while running Vewd internal test suites on
top of chromium 86.0.4240.x. I haven't managed to reproduce this on
content_shell but since I believe I do understand the root cause pretty
well I can say this also potentially affects chrome.

What is also interesting about this is the fact that the test case which
reproduced this quite consistently stopped doing so when using ASAN
enabled build. This leads me to believe there has to be some timing
factor involved here.

As far as I can tell this was introduced recently by commit
4b7b3265, "Convert SyncHandleRegistry to
use a CallbackList".

The problem happens sometimes when SyncEventWatcher is destroyed.
If we look at the declaration of this class we may notice that its
registry_ member variable will be destroyed before subscription_.
(NOTE: SyncEventRegistry::EventCallbackSubscription is a typedef to
SyncEventRegistry::Subscription). The instance of Subscription is created
in SyncEventRegistry::RegisterEvent. This function, among other things,
creates binding for "remove_closure" lambda function. This binding stores
a raw pointer to a EventCallbackList which is a raw pointer to a unique_ptr
value stored in SyncHandleRegistry::events_.

Given this information we can have the following scenario when the
SyncEventWatcher is destroyed:
1. SyncEventWatcher DTOR calls SyncEventRegistry DTOR.
2. SyncEventRegitry DTOR destroys its member variables and among them
   events_ which destroys all the EventCallbackList values storred in
   this map.
3. SyncEventWatcher DTOR calls SyncHandlerRegistry::Subscription DTOR
   which in turn calls its remove_runner_ closure we created in
   SyncHandleRegistry::RegisterEvent.
4. The remove closure function (lambda function defined in
   SyncHandleRegistry::RegisterEvent) tries to use EventCallbackList
   (first argument, stored in a binding) we destroyed in step 2 leading
   to a use-after-free bug.

The simplest fix is to reorder registry_ and subscription_ variables in
SyncEventWatcher to ensure the registry is always destroyed after
subscription.

Bug: 1133668
Change-Id: I35be7818b184308ccfb7a01895062de087233abf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2438399
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812176}

mojo/public/cpp/bindings/sync_event_watcher.h

@@ -53,13 +53,15 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) SyncEventWatcher {
   base::WaitableEvent* const event_;
   const base::RepeatingClosure callback_;
 
+  // Must outlive (and thus be declared before) |subscription_|, since
+  // it subscribes to a callback list stored in the registry.
+  scoped_refptr<SyncHandleRegistry> registry_;
+
   SyncHandleRegistry::EventCallbackSubscription subscription_;
 
   // If non-zero, |event_| should be registered with SyncHandleRegistry.
   size_t register_request_count_ = 0;
 
-  scoped_refptr<SyncHandleRegistry> registry_;
-
   scoped_refptr<base::RefCountedData<bool>> destroyed_;
 
   SEQUENCE_CHECKER(sequence_checker_);