|
|
Created:
4 years ago by Zhongyi Shi Modified:
4 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner.
HttpServerPropertiesManager used to get pref_task_runner by calling base::ThreadTaskRunnerHandle::Get() itself,
now changes to take the task runner passed in. No functional change.
There are two SingleThreadTaskRunners in HttpServerPropertiesManager:
pref_task_runner: which is used to post cache update on pref thread, and should be registered with pref_cache_update_timer_;
network_task_runner: which is used to post pref update on network thread, and should be registered with network_prefs_update_timer.
BUG=670519
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Committed: https://crrev.com/894658381ab6a524afa5ab08c780f8b4e4f3cb01
Cr-Commit-Position: refs/heads/master@{#439242}
Patch Set 1 #
Total comments: 8
Patch Set 2 : add comments to HttpServerPropertiesManager #
Dependent Patchsets: Messages
Total messages: 57 (37 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
Description was changed from ========== Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner BUG=670519 ========== to ========== Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner BUG=670519 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:120001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
zhongyi@chromium.org changed reviewers: + rch@chromium.org, xunjieli@chromium.org
Next cl will change the pref_task_runner in HttpServerPropertiesManager to TestMockTimeTaskRunner.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
zhongyi@chromium.org changed reviewers: + brettw@chromium.org
xunjieli: please take a look components/cronet. brettw: chrome/browser and ios/chrome/browser, please!
https://codereview.chromium.org/2567893002/diff/140001/net/http/http_server_p... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/2567893002/diff/140001/net/http/http_server_p... net/http/http_server_properties_manager.h:84: // Must be constructed on the Pref thread. Can you add a description here of the two task runners, why you need two, and what each one will be used for. I actually don't understand myself so I'll re-review after that. Thanks!
https://codereview.chromium.org/2567893002/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2567893002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:597: base::ThreadTaskRunnerHandle::Get(), GetNetworkTaskRunner())); Hmm.. This method is invoked on the network thread. base::ThreadTaskRunnerHandle::Get() is essentially GetNetworkTaskRunner(). I know this CL doesn't introduce any functional change, but my question is what should we use for |pref_task_runner_|? Is it bad for it to be the same as |network_task_runner_|?
https://codereview.chromium.org/2567893002/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2567893002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:597: base::ThreadTaskRunnerHandle::Get(), GetNetworkTaskRunner())); On 2016/12/14 01:47:11, xunjieli wrote: > Hmm.. This method is invoked on the network thread. > base::ThreadTaskRunnerHandle::Get() is essentially GetNetworkTaskRunner(). I > know this CL doesn't introduce any functional change, but my question is what > should we use for |pref_task_runner_|? Is it bad for it to be the same as > |network_task_runner_|? Yeah, this CL shouldn't introduce functional change. If I read the code correctly (you might also want to confirm), in src/chrome/browser, the HttpServerPropertiesManager is created by ProfileImplIOData::Handle::Init where it should be running on the BrowserThread::UI thread. I agreed that base::ThreaTaskRunnerHanlde::Get() is actually getting a network task runner as it's running on the network thread. Do you think this is a bug?
https://codereview.chromium.org/2567893002/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2567893002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:597: base::ThreadTaskRunnerHandle::Get(), GetNetworkTaskRunner())); On 2016/12/14 04:20:43, Zhongyi Shi wrote: > On 2016/12/14 01:47:11, xunjieli wrote: > > Hmm.. This method is invoked on the network thread. > > base::ThreadTaskRunnerHandle::Get() is essentially GetNetworkTaskRunner(). I > > know this CL doesn't introduce any functional change, but my question is what > > should we use for |pref_task_runner_|? Is it bad for it to be the same as > > |network_task_runner_|? > > Yeah, this CL shouldn't introduce functional change. If I read the code > correctly (you might also want to confirm), in src/chrome/browser, the > HttpServerPropertiesManager is created by ProfileImplIOData::Handle::Init where > it should be running on the BrowserThread::UI thread. I agreed that > base::ThreaTaskRunnerHanlde::Get() is actually getting a network task runner as > it's running on the network thread. Do you think this is a bug? I think the bug might be in HttpServerPropertiesManager. It's not clear to me why HttpServerPropertiesManager needs to receive notifications on UI thread. Maybe the PrefService has such limitation? I found one line on thread safety in pref_value_store.h but not anywhere else. It will be good to find out why we have this separation (pref_task_runner_ vs network_task_runner_). I talked to Bence offline as well. He wasn't sure about reason behind this setup. In Cronet, this will basically mean that we are posting tasks from the network thread to itself and back, which seems wasteful (but not dangerous). However, in Chrome, if this class starts to post too many tasks to the pref_task_runner_, we will be causing UI jank which seems dangerous and possible. Since this CL introduces no functional change, lgtm.
Description was changed from ========== Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner BUG=670519 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner. HttpServerPropertiesManager used to get pref_task_runner by calling base::ThreadTaskRunnerHandle::Get() itself, now changes to take the task runner passed in. No functional change. There are two SingleThreadTaskRunners in HttpServerPropertiesManager: pref_task_runner: which is used to post cache update on pref thread, and should be registered with pref_cache_update_timer_; network_task_runner: which is used to post pref update on network thread, and should be registered with network_prefs_update_timer. BUG=670519 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner. HttpServerPropertiesManager used to get pref_task_runner by calling base::ThreadTaskRunnerHandle::Get() itself, now changes to take the task runner passed in. No functional change. There are two SingleThreadTaskRunners in HttpServerPropertiesManager: pref_task_runner: which is used to post cache update on pref thread, and should be registered with pref_cache_update_timer_; network_task_runner: which is used to post pref update on network thread, and should be registered with network_prefs_update_timer. BUG=670519 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner. HttpServerPropertiesManager used to get pref_task_runner by calling base::ThreadTaskRunnerHandle::Get() itself, now changes to take the task runner passed in. No functional change. There are two SingleThreadTaskRunners in HttpServerPropertiesManager: pref_task_runner: which is used to post cache update on pref thread, and should be registered with pref_cache_update_timer_; network_task_runner: which is used to post pref update on network thread, and should be registered with network_prefs_update_timer. BUG=670519 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Thanks for helping the review, updated the cl description, ptal! https://codereview.chromium.org/2567893002/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2567893002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:597: base::ThreadTaskRunnerHandle::Get(), GetNetworkTaskRunner())); On 2016/12/14 18:49:38, xunjieli wrote: > I think the bug might be in HttpServerPropertiesManager. It's not clear to me > why HttpServerPropertiesManager needs to receive notifications on UI thread. > Maybe the PrefService has such limitation? I found one line on thread safety in > pref_value_store.h but not anywhere else. It will be good to find out why we > have this separation (pref_task_runner_ vs network_task_runner_). If you look at the HttpServerPropertiesManager documentation, it interacts with pref thread(on desktop, it's the UI thread) and network thread. I have also updated the cl description explaining why there're two task_runners in this class. However, note it's also required that "HttpServerPropertiesManager is constructed by on the pref thread, to set up |pref_task_runner_| and the prefs listeners". In cronet case, it's just all handed to networking thread and all the data are accessible by the networking thread. I talked to Ryan offline, and he felt it should be okay. > However, in Chrome, if this class starts to post too many tasks to the > pref_task_runner_, we will be causing UI jank which seems dangerous and > possible. We used timer to schedule posts every 60s so that we don't post too often. > Since this CL introduces no functional change, lgtm. Agreed that this CL shouldn't be blocked by the discussion here. It might get your attention that in cronet, network_task_runner and pref_task_runner are the same though. https://codereview.chromium.org/2567893002/diff/140001/net/http/http_server_p... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/2567893002/diff/140001/net/http/http_server_p... net/http/http_server_properties_manager.h:84: // Must be constructed on the Pref thread. On 2016/12/13 23:26:13, brettw (ping after 24h) wrote: > Can you add a description here of the two task runners, why you need two, and > what each one will be used for. I actually don't understand myself so I'll > re-review after that. Thanks! Sorry about the confusion. I have updated the CL description, ptal. Hope that helps reviewing.
https://codereview.chromium.org/2567893002/diff/140001/net/http/http_server_p... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/2567893002/diff/140001/net/http/http_server_p... net/http/http_server_properties_manager.h:84: // Must be constructed on the Pref thread. Thanks for the description update. But I think we need to update this comment as well. Somebody calling this function isn't going to find this CL description to see why there are two task runners and what each one should be.
Update: add comments in HttpServerPropertiesManager header file to document the usage of two task runners. ptal! https://codereview.chromium.org/2567893002/diff/140001/net/http/http_server_p... File net/http/http_server_properties_manager.h (right): https://codereview.chromium.org/2567893002/diff/140001/net/http/http_server_p... net/http/http_server_properties_manager.h:84: // Must be constructed on the Pref thread. On 2016/12/14 23:16:54, brettw (ping after 24h) wrote: > Thanks for the description update. But I think we need to update this comment as > well. Somebody calling this function isn't going to find this CL description to > see why there are two task runners and what each one should be. Oh, sorry that I miss the comments in the headers files. Updated now.
lgtm
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org, xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2567893002/#ps160001 (title: "add comments to HttpServerPropertiesManager")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zhongyi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zhongyi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zhongyi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481912383025610, "parent_rev": "1dd4559b2eefa25c3df2c8fe3e218fecc7a2848a", "commit_rev": "e4f7fd4296803c7615be2707d5c4ae5e0f2a1b65"}
Message was sent while issue was closed.
Description was changed from ========== Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner. HttpServerPropertiesManager used to get pref_task_runner by calling base::ThreadTaskRunnerHandle::Get() itself, now changes to take the task runner passed in. No functional change. There are two SingleThreadTaskRunners in HttpServerPropertiesManager: pref_task_runner: which is used to post cache update on pref thread, and should be registered with pref_cache_update_timer_; network_task_runner: which is used to post pref update on network thread, and should be registered with network_prefs_update_timer. BUG=670519 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner. HttpServerPropertiesManager used to get pref_task_runner by calling base::ThreadTaskRunnerHandle::Get() itself, now changes to take the task runner passed in. No functional change. There are two SingleThreadTaskRunners in HttpServerPropertiesManager: pref_task_runner: which is used to post cache update on pref thread, and should be registered with pref_cache_update_timer_; network_task_runner: which is used to post pref update on network thread, and should be registered with network_prefs_update_timer. BUG=670519 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2567893002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner. HttpServerPropertiesManager used to get pref_task_runner by calling base::ThreadTaskRunnerHandle::Get() itself, now changes to take the task runner passed in. No functional change. There are two SingleThreadTaskRunners in HttpServerPropertiesManager: pref_task_runner: which is used to post cache update on pref thread, and should be registered with pref_cache_update_timer_; network_task_runner: which is used to post pref update on network thread, and should be registered with network_prefs_update_timer. BUG=670519 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2567893002 ========== to ========== Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a TestMockTimeTaskRunner. HttpServerPropertiesManager used to get pref_task_runner by calling base::ThreadTaskRunnerHandle::Get() itself, now changes to take the task runner passed in. No functional change. There are two SingleThreadTaskRunners in HttpServerPropertiesManager: pref_task_runner: which is used to post cache update on pref thread, and should be registered with pref_cache_update_timer_; network_task_runner: which is used to post pref update on network thread, and should be registered with network_prefs_update_timer. BUG=670519 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Committed: https://crrev.com/894658381ab6a524afa5ab08c780f8b4e4f3cb01 Cr-Commit-Position: refs/heads/master@{#439242} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/894658381ab6a524afa5ab08c780f8b4e4f3cb01 Cr-Commit-Position: refs/heads/master@{#439242} |