BrowserThreadTaskRunner::RunsTasksOnCurrentThread is wrong |
|||
Issue descriptionBrowserThreadTaskRunner::RunsTasksOnCurrentThread() relies on the global BrowserThread table which has been cleared by the time WillDestroyCurrentMessageLoop() is run on MessageLoop DestructionObservers. This means that any such observers which happen to have a BrowserThread TaskRunner cached can not rely on the result of this call being correct. This has caused subtle and confusing problems for me and will likely do the same for others, so we should fix it.
,
Jul 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dd05854bb54363ce7d57ad1bedfe8df33ad76191 commit dd05854bb54363ce7d57ad1bedfe8df33ad76191 Author: rockot <rockot@chromium.org> Date: Thu Jul 28 02:04:24 2016 Revert of Ensure BrowserThread::CurrentlyOn is correct through MessageLoop teardown (patchset #8 id:160001 of https://codereview.chromium.org/2180253003/ ) Reason for revert: Test failures in the changes to the ComponentCloudPolicyTest. Doh. Original issue's description: > Ensure BrowserThread::CurrentlyOn is correct through MessageLoop teardown > > Changes BrowserThread::CurrentlyOn (and thus > BrowserThreadTaskRunner::RunsTasksOnCurrentThread()) to correctly report > a BrowserThread's association during MessageLoop destruction notification. > > Also adds an explicit Start() to BrowserThreadImpl as there are tests which > call it and which incorrectly assumed base::Thread's implementation would > call BrowserThreadImpl::StartWithOptions (which it wouldn't since the latter > is not a virtual function.) > > This change provokes many tests to delete ExtensionFunction instances that > were otherwise being leaked because UIThreadExtensionFunction::Destroy was > incorrectly deferring destruction when run on the UI thread during shutdown. > This in turn revealed a few small bugs which have also been fixed here. > > BUG= 631093 > R=jam@chromium.org > > Committed: https://crrev.com/b02da29fb9116d1a1fb4fd0476628f333ff6bd1a > Cr-Commit-Position: refs/heads/master@{#408295} TBR=jam@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 631093 Review-Url: https://codereview.chromium.org/2186213002 Cr-Commit-Position: refs/heads/master@{#408311} [modify] https://crrev.com/dd05854bb54363ce7d57ad1bedfe8df33ad76191/chrome/browser/browser_process_impl.cc [modify] https://crrev.com/dd05854bb54363ce7d57ad1bedfe8df33ad76191/chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc [modify] https://crrev.com/dd05854bb54363ce7d57ad1bedfe8df33ad76191/chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc [modify] https://crrev.com/dd05854bb54363ce7d57ad1bedfe8df33ad76191/chrome/browser/safe_browsing/protocol_manager_unittest.cc [modify] https://crrev.com/dd05854bb54363ce7d57ad1bedfe8df33ad76191/content/browser/browser_thread_impl.cc [modify] https://crrev.com/dd05854bb54363ce7d57ad1bedfe8df33ad76191/content/browser/browser_thread_impl.h [modify] https://crrev.com/dd05854bb54363ce7d57ad1bedfe8df33ad76191/content/browser/browser_thread_unittest.cc [modify] https://crrev.com/dd05854bb54363ce7d57ad1bedfe8df33ad76191/extensions/browser/extension_function.cc
,
Jul 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/48a6aacd388c57b77d037e21bffe64a0c8c78b39 commit 48a6aacd388c57b77d037e21bffe64a0c8c78b39 Author: rockot <rockot@chromium.org> Date: Thu Jul 28 17:23:54 2016 Ensure BrowserThread::CurrentlyOn is correct through MessageLoop teardown Changes BrowserThread::CurrentlyOn (and thus BrowserThreadTaskRunner::RunsTasksOnCurrentThread()) to correctly report a BrowserThread's association during MessageLoop destruction notification. Also adds an explicit Start() to BrowserThreadImpl as there are tests which call it and which incorrectly assumed base::Thread's implementation would call BrowserThreadImpl::StartWithOptions (which it wouldn't since the latter is not a virtual function.) This change provokes many tests to delete ExtensionFunction instances that were otherwise being leaked because UIThreadExtensionFunction::Destroy was incorrectly deferring destruction when run on the UI thread during shutdown. This in turn revealed a few small bugs which have also been fixed here. BUG= 631093 R=jam@chromium.org Committed: https://crrev.com/b02da29fb9116d1a1fb4fd0476628f333ff6bd1a Review-Url: https://codereview.chromium.org/2180253003 Cr-Original-Commit-Position: refs/heads/master@{#408295} Cr-Commit-Position: refs/heads/master@{#408411} [modify] https://crrev.com/48a6aacd388c57b77d037e21bffe64a0c8c78b39/chrome/browser/browser_process_impl.cc [modify] https://crrev.com/48a6aacd388c57b77d037e21bffe64a0c8c78b39/chrome/browser/policy/cloud/component_cloud_policy_browsertest.cc [modify] https://crrev.com/48a6aacd388c57b77d037e21bffe64a0c8c78b39/chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc [modify] https://crrev.com/48a6aacd388c57b77d037e21bffe64a0c8c78b39/chrome/browser/safe_browsing/protocol_manager_unittest.cc [modify] https://crrev.com/48a6aacd388c57b77d037e21bffe64a0c8c78b39/content/browser/browser_thread_impl.cc [modify] https://crrev.com/48a6aacd388c57b77d037e21bffe64a0c8c78b39/content/browser/browser_thread_impl.h [modify] https://crrev.com/48a6aacd388c57b77d037e21bffe64a0c8c78b39/content/browser/browser_thread_unittest.cc [modify] https://crrev.com/48a6aacd388c57b77d037e21bffe64a0c8c78b39/extensions/browser/extension_function.cc
,
Jul 28 2016
,
Nov 4 2016
Issue 55774 has been merged into this issue. |
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Jul 28 2016