Invalidate WeakPtrs of ResourceMessageFilter on channel shutdown

content/browser/loader/resource_message_filter_unittest.cc

Index: content/browser/loader/resource_message_filter_unittest.cc
diff --git a/content/browser/loader/resource_message_filter_unittest.cc b/content/browser/loader/resource_message_filter_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c0106ae463b8ec22541f6f4fe9a4291050eeac9d
--- /dev/null
+++ b/content/browser/loader/resource_message_filter_unittest.cc
@@ -0,0 +1,87 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/loader/resource_message_filter.h"
+
+#include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
+#include "base/synchronization/waitable_event.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/common/process_type.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+
+void TakeWeakPtrOnIOThread(ResourceMessageFilter* resource_message_filter,
+                           base::WeakPtr<ResourceMessageFilter>* weak_ptr) {
+  *weak_ptr = resource_message_filter->GetWeakPtr();
+}
+
+void AssertNonNullOnIOThread(base::WeakPtr<ResourceMessageFilter> weak_ptr) {
+  ASSERT_TRUE(weak_ptr);
+}
+
+void AssertNullOnIOThread(base::WeakPtr<ResourceMessageFilter> weak_ptr) {
+  ASSERT_FALSE(weak_ptr);
+}
+
+void WaitForSignalAndCreateStrongReference(
+    base::WaitableEvent* waitable_event,
+    base::WeakPtr<ResourceMessageFilter> weak_ptr) {
+  waitable_event->Wait();
+  scoped_refptr<ResourceMessageFilter> strong_reference(weak_ptr.get());
+  EXPECT_FALSE(strong_reference);

[mmenke, 2016/11/09 18:34:19]

Is this materially different from EXPECT_FALSE(weak_ptr.get())?

[tzik, 2016/11/10 06:08:14]

Added a comment here.
No, they are equivalent here. But, since my purpose is to ensure the strong
reference doesn't cause a crash, EXPECT_FALSE(strong_reference); is probably
more straightforward.

+}
+
+void RunOnIOThread(const base::Closure& closure) {
+  base::RunLoop run_loop;
+  BrowserThread::PostTaskAndReply(BrowserThread::IO, FROM_HERE, closure,
+                                  run_loop.QuitClosure());
+  run_loop.Run();
+}
+
+TEST(ResourceMessageFilterTest, InvalidateOnChannelShutdown) {
+  TestBrowserThreadBundle thread_bundle(
+      TestBrowserThreadBundle::REAL_IO_THREAD);
+  ResourceDispatcherHostImpl resource_dispatcher_host;
+
+  scoped_refptr<ResourceMessageFilter> resource_message_filter(
+      new ResourceMessageFilter(1, PROCESS_TYPE_RENDERER, nullptr, nullptr,
+                                nullptr, nullptr,
+                                ResourceMessageFilter::GetContextsCallback()));
+
+  base::WeakPtr<ResourceMessageFilter> weak_ptr;
+  RunOnIOThread(base::Bind(&TakeWeakPtrOnIOThread,
+                           base::Unretained(resource_message_filter.get()),
+                           base::Unretained(&weak_ptr)));
+  RunOnIOThread(base::Bind(&AssertNonNullOnIOThread, weak_ptr));
+
+  RunOnIOThread(base::Bind(&ResourceMessageFilter::OnChannelClosing,
+                           base::Unretained(resource_message_filter.get())));
+
+  base::WaitableEvent waitable_event(
+      base::WaitableEvent::ResetPolicy::MANUAL,
+      base::WaitableEvent::InitialState::NOT_SIGNALED);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(&WaitForSignalAndCreateStrongReference,
+                                     &waitable_event, weak_ptr));
+
+  // The last strong reference to |resource_message_filter| is cleared here. The
+  // deletion handler should posts a task to IO thread to delete
+  // |resource_message_filter| on it. Then, before the task is arrived to IO
+  // thread, a task posted above makes a strong reference from a WeakPtr.
+  // Ensure the WeakPtr is invalidated at that point, and that doesn't cause a
+  // crash.
+  DCHECK(resource_message_filter->HasOneRef());
+  resource_message_filter = nullptr;
+
+  waitable_event.Signal();
+
+  resource_dispatcher_host.Shutdown();

[mmenke, 2016/11/09 18:34:19]

Why do we need a resource_dispatcher_host?  I'm really not following what this
test is doing, or what resource_message_filter has to do with the RDH in this
test.

[tzik, 2016/11/10 06:08:14]

This is used by the impl of ResourceMessageFilter::OnChannelClosed, it touchs
the ResourceDispatcherHost instance taken from a global variable.

+  RunOnIOThread(base::Bind(&base::DoNothing));

[mmenke, 2016/11/09 18:34:19]

Why is this needed?

[tzik, 2016/11/10 06:08:14]

This is to wait for ResourceMessageFilter destruction on IO thread. I think
ResourceDispatcherHost should conceptually outlive ResourceMessageFilter, and
without this RunOnIOThread, the destruction order of ResourceDispatcherHost and
ResourceMessageFilter is undefined.

+}
+
+}  // namespace content