Pass pref_task_runner through HttpServerPropertiesManager's cxtor so that tests could inject a Test…

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}

[Zhongyi Shi, 2016-12-11 05:14:11]

Patchset #1 (id:1) has been deleted

[Zhongyi Shi, 2016-12-11 05:14:31]

The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 05:14:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhongyi Shi, 2016-12-11 07:30:57]

The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 07:31:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-11 07:38:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-11 07:38:03]

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...)

[Zhongyi Shi, 2016-12-12 05:48:52]

The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 05:49:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 06:12:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 06:12:12]

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...)

[Zhongyi Shi, 2016-12-13 19:54:50]

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
==========

[Zhongyi Shi, 2016-12-13 19:55:00]

The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 19:55:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhongyi Shi, 2016-12-13 21:24:38]

Patchset #6 (id:120001) has been deleted

[Zhongyi Shi, 2016-12-13 21:24:46]

Patchset #2 (id:40001) has been deleted

[Zhongyi Shi, 2016-12-13 21:28:32]

Patchset #4 (id:100001) has been deleted

[Zhongyi Shi, 2016-12-13 21:28:40]

Patchset #1 (id:20001) has been deleted

[Zhongyi Shi, 2016-12-13 21:28:47]

Patchset #1 (id:60001) has been deleted

[Zhongyi Shi, 2016-12-13 21:37:24]

Patchset #1 (id:80001) has been deleted

[Zhongyi Shi, 2016-12-13 21:39:04]

zhongyi@chromium.org changed reviewers:
+ rch@chromium.org, xunjieli@chromium.org

[Zhongyi Shi, 2016-12-13 21:39:05]

Next cl will change the pref_task_runner in HttpServerPropertiesManager to
TestMockTimeTaskRunner.

[commit-bot: I haz the power, 2016-12-13 22:33:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 22:33:12]

Dry run: This issue passed the CQ dry run.

[Ryan Hamilton, 2016-12-13 22:44:36]

lgtm

[Zhongyi Shi, 2016-12-13 23:03:10]

zhongyi@chromium.org changed reviewers:
+ brettw@chromium.org

[Zhongyi Shi, 2016-12-13 23:03:10]

xunjieli: 
please take a look components/cronet. 

brettw:
chrome/browser and ios/chrome/browser, please!

[brettw, 2016-12-13 23:26:13]

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!

[xunjieli, 2016-12-14 01:47:11]

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_|?

[Zhongyi Shi, 2016-12-14 04:20:43]

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?

[xunjieli, 2016-12-14 18:49:38]

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.

[Zhongyi Shi, 2016-12-14 19:29:33]

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
==========

[Zhongyi Shi, 2016-12-14 19:29:56]

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
==========

[Zhongyi Shi, 2016-12-14 19:47:25]

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.

[brettw, 2016-12-14 23:16:54]

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.

[Zhongyi Shi, 2016-12-14 23:36:58]

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.

[brettw, 2016-12-15 17:30:16]

lgtm

[Zhongyi Shi, 2016-12-15 18:54:19]

The CQ bit was checked by zhongyi@chromium.org

[Zhongyi Shi, 2016-12-15 18:54:19]

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")

[commit-bot: I haz the power, 2016-12-15 18:54:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 20:22:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 20:22:26]

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_...)

[Zhongyi Shi, 2016-12-15 21:04:18]

The CQ bit was checked by zhongyi@chromium.org

[commit-bot: I haz the power, 2016-12-15 21:04:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 22:40:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 22:41:01]

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_...)

[Zhongyi Shi, 2016-12-15 23:09:43]

The CQ bit was checked by zhongyi@chromium.org

[commit-bot: I haz the power, 2016-12-15 23:11:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 02:56:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 02:56:51]

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_...)

[Zhongyi Shi, 2016-12-16 18:19:43]

The CQ bit was checked by zhongyi@chromium.org

[commit-bot: I haz the power, 2016-12-16 18:20:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 00:08:04]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481912383025610,
"parent_rev": "1dd4559b2eefa25c3df2c8fe3e218fecc7a2848a", "commit_rev":
"e4f7fd4296803c7615be2707d5c4ae5e0f2a1b65"}

[commit-bot: I haz the power, 2016-12-17 00:08:33]

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
==========

[commit-bot: I haz the power, 2016-12-17 00:08:34]

Committed patchset #2 (id:160001)

[commit-bot: I haz the power, 2016-12-17 00:11:10]

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}
==========

[commit-bot: I haz the power, 2016-12-17 00:11:11]

Patchset 2 (id:??) landed as
https://crrev.com/894658381ab6a524afa5ab08c780f8b4e4f3cb01
Cr-Commit-Position: refs/heads/master@{#439242}