Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2115823004:
Avoid using BrowserThread and ContentBrowserClient in NetLogObserver. (Closed)
|
Created:
4 years, 5 months ago by ananta Modified:
4 years, 5 months ago CC:
chromium-reviews, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, mmenke, devtools-reviews_chromium.org, loading-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid using BrowserThread and ContentBrowserClient in NetLogObserver.
To avoid using the BrowserThread we instantiate a ThreadChecker instance on the NetLogObserver::Attach method and use this to validate that calls to the NetLogObserver instance occur on the same thread.
To avoid using ContentBrowserClient, we pass the NetLog pointer in the NetLogObserver::Attach method.
BUG=598073
Committed: https://crrev.com/92e2691c3adb201ba8889d6050ed34cbce8184bf
Cr-Commit-Position: refs/heads/master@{#405284}
Patch Set 1 #Patch Set 2 : Fix build error #
Total comments: 4
Patch Set 3 : Pass the IO thread task runnner in NetLogObserver::Attach #Patch Set 4 : Fix build error #Patch Set 5 : Fix content_unittests redness #Patch Set 6 : Rebase to tip #
Total comments: 6
Patch Set 7 : Address review comments #Patch Set 8 : Fix build error #
Total comments: 2
Patch Set 9 : Ensure that NetLogObserver::GetInstance does not crash if it is called before Attach or after Detach #Patch Set 10 : Use ThreadChecker instead of task runner #Patch Set 11 : Remove include #
Total comments: 2
Patch Set 12 : Fix leak #
Messages
Total messages: 30 (10 generated)
ananta@chromium.org changed reviewers: + jam@chromium.org
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/2115823004/diff/20001/content/browser/devtool... File content/browser/devtools/devtools_manager.cc (right): https://codereview.chromium.org/2115823004/diff/20001/content/browser/devtool... content/browser/devtools/devtools_manager.cc:25: NetLogObserver::SetIOThreadTaskRunner( It is unclear why netlog observer is initialized with the thread from the devtools manager. The scope of netlog is greater than the one of the manager. https://codereview.chromium.org/2115823004/diff/20001/content/browser/loader/... File content/browser/loader/netlog_observer.cc (right): https://codereview.chromium.org/2115823004/diff/20001/content/browser/loader/... content/browser/loader/netlog_observer.cc:210: io_thread_task_runner_.Get() = io_thread_runner; Could you instantiate a thread checker in the ::Attach instead?
https://codereview.chromium.org/2115823004/diff/20001/content/browser/devtool... File content/browser/devtools/devtools_manager.cc (right): https://codereview.chromium.org/2115823004/diff/20001/content/browser/devtool... content/browser/devtools/devtools_manager.cc:25: NetLogObserver::SetIOThreadTaskRunner( On 2016/07/06 18:36:14, pfeldman wrote: > It is unclear why netlog observer is initialized with the thread from the > devtools manager. The scope of netlog is greater than the one of the manager. Passed it via Attach instead. https://codereview.chromium.org/2115823004/diff/20001/content/browser/loader/... File content/browser/loader/netlog_observer.cc (right): https://codereview.chromium.org/2115823004/diff/20001/content/browser/loader/... content/browser/loader/netlog_observer.cc:210: io_thread_task_runner_.Get() = io_thread_runner; On 2016/07/06 18:36:14, pfeldman wrote: > Could you instantiate a thread checker in the ::Attach instead? Done.
On 2016/07/08 14:11:02, ananta wrote: > https://codereview.chromium.org/2115823004/diff/20001/content/browser/devtool... > File content/browser/devtools/devtools_manager.cc (right): > > https://codereview.chromium.org/2115823004/diff/20001/content/browser/devtool... > content/browser/devtools/devtools_manager.cc:25: > NetLogObserver::SetIOThreadTaskRunner( > On 2016/07/06 18:36:14, pfeldman wrote: > > It is unclear why netlog observer is initialized with the thread from the > > devtools manager. The scope of netlog is greater than the one of the manager. > > Passed it via Attach instead. > > https://codereview.chromium.org/2115823004/diff/20001/content/browser/loader/... > File content/browser/loader/netlog_observer.cc (right): > > https://codereview.chromium.org/2115823004/diff/20001/content/browser/loader/... > content/browser/loader/netlog_observer.cc:210: io_thread_task_runner_.Get() = > io_thread_runner; > On 2016/07/06 18:36:14, pfeldman wrote: > > Could you instantiate a thread checker in the ::Attach instead? > > Done. Please hold off on reviewing this. Looking at the failures.
On 2016/07/08 15:31:13, ananta wrote: > On 2016/07/08 14:11:02, ananta wrote: > > > https://codereview.chromium.org/2115823004/diff/20001/content/browser/devtool... > > File content/browser/devtools/devtools_manager.cc (right): > > > > > https://codereview.chromium.org/2115823004/diff/20001/content/browser/devtool... > > content/browser/devtools/devtools_manager.cc:25: > > NetLogObserver::SetIOThreadTaskRunner( > > On 2016/07/06 18:36:14, pfeldman wrote: > > > It is unclear why netlog observer is initialized with the thread from the > > > devtools manager. The scope of netlog is greater than the one of the > manager. > > > > Passed it via Attach instead. > > > > > https://codereview.chromium.org/2115823004/diff/20001/content/browser/loader/... > > File content/browser/loader/netlog_observer.cc (right): > > > > > https://codereview.chromium.org/2115823004/diff/20001/content/browser/loader/... > > content/browser/loader/netlog_observer.cc:210: io_thread_task_runner_.Get() = > > io_thread_runner; > > On 2016/07/06 18:36:14, pfeldman wrote: > > > Could you instantiate a thread checker in the ::Attach instead? > > > > Done. > > Please hold off on reviewing this. Looking at the failures. Please take a look. Thanks
> > Could you instantiate a thread checker in the ::Attach instead? > > Done. I don't see it in the patch set.
On 2016/07/11 20:56:15, pfeldman wrote: > > > Could you instantiate a thread checker in the ::Attach instead? > > > > Done. > > I don't see it in the patch set. The taskrunner for the IO thread is passed in via the Attach method. By instantiate the thread checker in Attach, did you mean checking whether it is invoked on the IO thread?
> The taskrunner for the IO thread is passed in via the Attach method. By > instantiate the thread checker in Attach, did you mean checking whether it is invoked on the IO thread? You pass the task runner, but you only use it as a thread checker. You could instead use a real thread checker that you instantiate in Attach so that it picked the invocation thread on its own. You would not need to pass the task runner into Attach then.
https://codereview.chromium.org/2115823004/diff/100001/content/browser/loader... File content/browser/loader/netlog_observer.cc (right): https://codereview.chromium.org/2115823004/diff/100001/content/browser/loader... content/browser/loader/netlog_observer.cc:52: void NetLogObserver::OnAddURLRequestEntry(const net::NetLog::Entry& entry) { nit: seems like this method could be private, then we could drop the check. https://codereview.chromium.org/2115823004/diff/100001/content/browser/loader... content/browser/loader/netlog_observer.cc:176: io_thread_task_runner_.Get()->BelongsToCurrentThread()); reset io_thread_task_runner_ (or checker) for symmetry https://codereview.chromium.org/2115823004/diff/100001/content/browser/loader... content/browser/loader/netlog_observer.cc:187: DCHECK(io_thread_task_runner_.Get().get() && This logic has changed - we used to return nullptr in case called before attach or after detach.
https://codereview.chromium.org/2115823004/diff/100001/content/browser/loader... File content/browser/loader/netlog_observer.cc (right): https://codereview.chromium.org/2115823004/diff/100001/content/browser/loader... content/browser/loader/netlog_observer.cc:52: void NetLogObserver::OnAddURLRequestEntry(const net::NetLog::Entry& entry) { On 2016/07/13 17:17:49, pfeldman wrote: > nit: seems like this method could be private, then we could drop the check. Done. https://codereview.chromium.org/2115823004/diff/100001/content/browser/loader... content/browser/loader/netlog_observer.cc:176: io_thread_task_runner_.Get()->BelongsToCurrentThread()); On 2016/07/13 17:17:49, pfeldman wrote: > reset io_thread_task_runner_ (or checker) for symmetry Done. https://codereview.chromium.org/2115823004/diff/100001/content/browser/loader... content/browser/loader/netlog_observer.cc:187: DCHECK(io_thread_task_runner_.Get().get() && On 2016/07/13 17:17:49, pfeldman wrote: > This logic has changed - we used to return nullptr in case called before attach > or after detach. Please clarify. The only change in this patch is to change the DCHECK? The instance_ will be null if we call this before Attach or after Detach
Also, you really want a ThreadChecker instead of passing the task runner. https://codereview.chromium.org/2115823004/diff/140001/content/browser/loader... File content/browser/loader/netlog_observer.cc (right): https://codereview.chromium.org/2115823004/diff/140001/content/browser/loader... content/browser/loader/netlog_observer.cc:185: DCHECK(io_thread_task_runner_.Get().get() && Before the change: calling NetLogObserver::GetInstance() before NetLogObserver::Attach() returned nullptr. After the change: calling NetLogObserver::GetInstance() before NetLogObserver::Attach() crashes.
https://codereview.chromium.org/2115823004/diff/140001/content/browser/loader... File content/browser/loader/netlog_observer.cc (right): https://codereview.chromium.org/2115823004/diff/140001/content/browser/loader... content/browser/loader/netlog_observer.cc:185: DCHECK(io_thread_task_runner_.Get().get() && On 2016/07/13 18:16:19, pfeldman wrote: > Before the change: calling NetLogObserver::GetInstance() before > NetLogObserver::Attach() returned nullptr. > After the change: calling NetLogObserver::GetInstance() before > NetLogObserver::Attach() crashes. Fixed this. Changed the task runner to a thread checker static instance.
Description was changed from ========== Avoid using BrowserThread and ContentBrowserClient in NetLogObserver. To avoid using the BrowserThread we pass the IO thread task runner to the NetLogObserver class via a new static method SetIOThreadTaskRunner. To avoid using ContentBrowserClient, we pass the NetLog pointer in the NetLogObserver::Attach method. BUG=598073 ========== to ========== Avoid using BrowserThread and ContentBrowserClient in NetLogObserver. To avoid using the BrowserThread we instantiate a ThreadChecker instance on the NetLogObserver::Attach method and use this to validate that calls to the NetLogObserver instance occur on the same thread. To avoid using ContentBrowserClient, we pass the NetLog pointer in the NetLogObserver::Attach method. BUG=598073 ==========
Description was changed from ========== Avoid using BrowserThread and ContentBrowserClient in NetLogObserver. To avoid using the BrowserThread we instantiate a ThreadChecker instance on the NetLogObserver::Attach method and use this to validate that calls to the NetLogObserver instance occur on the same thread. To avoid using ContentBrowserClient, we pass the NetLog pointer in the NetLogObserver::Attach method. BUG=598073 ========== to ========== Avoid using BrowserThread and ContentBrowserClient in NetLogObserver. To avoid using the BrowserThread we instantiate a ThreadChecker instance on the NetLogObserver::Attach method and use this to validate that calls to the NetLogObserver instance occur on the same thread. To avoid using ContentBrowserClient, we pass the NetLog pointer in the NetLogObserver::Attach method. BUG=598073 ==========
lgtm % comment https://codereview.chromium.org/2115823004/diff/200001/content/browser/loader... File content/browser/loader/netlog_observer.cc (right): https://codereview.chromium.org/2115823004/diff/200001/content/browser/loader... content/browser/loader/netlog_observer.cc:172: io_thread_checker_.Get().release(); .reset, otherwise you leak it.
https://codereview.chromium.org/2115823004/diff/200001/content/browser/loader... File content/browser/loader/netlog_observer.cc (right): https://codereview.chromium.org/2115823004/diff/200001/content/browser/loader... content/browser/loader/netlog_observer.cc:172: io_thread_checker_.Get().release(); On 2016/07/13 19:28:09, pfeldman wrote: > .reset, otherwise you leak it. Done.
The CQ bit was checked by ananta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/2115823004/#ps220001 (title: "Fix leak")
The CQ bit was unchecked by ananta@chromium.org
The CQ bit was checked by ananta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Avoid using BrowserThread and ContentBrowserClient in NetLogObserver. To avoid using the BrowserThread we instantiate a ThreadChecker instance on the NetLogObserver::Attach method and use this to validate that calls to the NetLogObserver instance occur on the same thread. To avoid using ContentBrowserClient, we pass the NetLog pointer in the NetLogObserver::Attach method. BUG=598073 ========== to ========== Avoid using BrowserThread and ContentBrowserClient in NetLogObserver. To avoid using the BrowserThread we instantiate a ThreadChecker instance on the NetLogObserver::Attach method and use this to validate that calls to the NetLogObserver instance occur on the same thread. To avoid using ContentBrowserClient, we pass the NetLog pointer in the NetLogObserver::Attach method. BUG=598073 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Avoid using BrowserThread and ContentBrowserClient in NetLogObserver. To avoid using the BrowserThread we instantiate a ThreadChecker instance on the NetLogObserver::Attach method and use this to validate that calls to the NetLogObserver instance occur on the same thread. To avoid using ContentBrowserClient, we pass the NetLog pointer in the NetLogObserver::Attach method. BUG=598073 ========== to ========== Avoid using BrowserThread and ContentBrowserClient in NetLogObserver. To avoid using the BrowserThread we instantiate a ThreadChecker instance on the NetLogObserver::Attach method and use this to validate that calls to the NetLogObserver instance occur on the same thread. To avoid using ContentBrowserClient, we pass the NetLog pointer in the NetLogObserver::Attach method. BUG=598073 Committed: https://crrev.com/92e2691c3adb201ba8889d6050ed34cbce8184bf Cr-Commit-Position: refs/heads/master@{#405284} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/92e2691c3adb201ba8889d6050ed34cbce8184bf Cr-Commit-Position: refs/heads/master@{#405284}
Message was sent while issue was closed.
update content/browser/loader/DEPS?
Message was sent while issue was closed.
On 2016/07/14 20:12:52, jam wrote: > update content/browser/loader/DEPS? Will look when I get back on Monday. It seems like the files removed from the includes here are still used in other places in the content/browser/loader directory |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
