[Sync] Migrate SyncInternalsMessageHandler off CallJavascriptFunctionUnsafe.

[Sync] Migrate SyncInternalsMessageHandler off CallJavascriptFunctionUnsafe.

This CL is a result of comments in
https://codereview.chromium.org/2872023002/ that became too large to be
included in the original code review. These changes should make
SyncInternalsMessageHandler behave more safely in regards to when
javascript methods are invoked, as well as increasing test coverage.

* Replaced CallJavascriptFunctionUnsafe with CallJavascriptFunction.
This mostly worked by adding AllowJavascript to all callbacks from
javascript, and unregistering from model callbacks/observations in
OnJavascriptDisallowed(). However, this didn't work for
OnReceivedAllNodes, which is called asycronously. Instead we need to
manually verify IsJavascriptAllowed() is still true. 
* Moved SigninManagerBase accessor from ProfileSyncService to
SyncService[Base]. This allowed us to remove the SyncManagerBase param
from the ConstructAboutInformation function. In general we want to move
things out of ProfileSyncService that don't have a reason to be there.
* Reworked how SyncInternalsMessageHandler accesses a SyncService to
facilitate unit tests.
* Replaced extractor with callback to be more similar to SyncService
injection pattern.
* Added test coverage for registration/unregistration.

BUG=719044
Review-Url: https://codereview.chromium.org/2898723003
Cr-Commit-Position: refs/heads/master@{#474755}
Committed: https://chromium.googlesource.com/chromium/src/+/b522e5fd22c3454dff23fc48b43bafebd160a543

[skym, 2017-05-22 21:13:53]

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

[commit-bot: I haz the power, 2017-05-22 21:14:36]

Dry run: CQ is trying da patch.

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

[skym, 2017-05-22 21:22:07]

Description was changed from

==========
[Sync] Migrate SyncInternalsMessageHandler off CallJavascriptFunctionUnsafe.

BUG=719044
==========

to

==========
[Sync] Migrate SyncInternalsMessageHandler off CallJavascriptFunctionUnsafe.

This CL is a result of comments in
https://codereview.chromium.org/2872023002/ that became too large to be
included in the original code review. These changes should make
SyncInternalsMessageHandler behave more safely in regards to when
javascript methods are invoked, as well as increasing test coverage.

* Replaced CallJavascriptFunctionUnsafe with CallJavascriptFunction.
This mostly worked by adding AllowJavascript to all callbacks from
javascript, and unregistering from model callbacks/observations in
OnJavascriptDisallowed(). However, this didn't work for
OnReceivedAllNodes, which is called asycronously. Instead we need to
manually verify IsJavascriptAllowed() is still true. 
* Moved SigninManagerBase accessor from ProfileSyncService to
SyncService[Base]. This allowed us to remove the SyncManagerBase param
from the ConstructAboutInformation function. In general we want to move
things out of ProfileSyncService that don't have a reason to be there.
* Reworked how SyncInternalsMessageHandler accesses a SyncService to
facilitate unit tests.
* Replaced extractor with callback to be more similar to SyncService
injection pattern.
* Added test coverage for registration/unregistration.

BUG=719044
==========

[skym, 2017-05-22 21:22:08]

skym@chromium.org changed reviewers:
+ afakhry@chromium.org, dbeam@chromium.org, pnoland@chromium.org

[commit-bot: I haz the power, 2017-05-22 21:25:38]

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

[commit-bot: I haz the power, 2017-05-22 21:25:40]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[skym, 2017-05-22 21:30:17]

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

[commit-bot: I haz the power, 2017-05-22 21:31:06]

Dry run: CQ is trying da patch.

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

[skym, 2017-05-22 21:55:44]

PTAL
afakhry@ for
chrome/browser/feedback/system_logs/log_sources/chrome_internal_log_source.cc
dbeam@ for chrome/browser/ui/webui/sync_internals_message_handler.*
pnoland@ for everything else.

[commit-bot: I haz the power, 2017-05-22 22:01:13]

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

[commit-bot: I haz the power, 2017-05-22 22:01:14]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[skym, 2017-05-22 22:04:50]

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

[commit-bot: I haz the power, 2017-05-22 22:05:32]

Dry run: CQ is trying da patch.

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

[afakhry, 2017-05-22 23:31:39]

feedback lgtm.

[Dan Beam, 2017-05-22 23:31:43]

https://codereview.chromium.org/2898723003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/2898723003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:58:
UnregisterModelNotifications();
can we just call weak_ptr_factory_.InvalidateWeakPtrs() here to sever the
connection to any async, long-living callbacks?  this doesn't really change much
about existing code and just ensures what when the page is refreshed or a
renderer dies, there will be no callbacks called until javascript is allowed
again.

if the JS (as it's initialized) always calls the same messages to get
information, disconnecting the weak pointers for existing calls and then
re-doing the work when the JS is created seems like the simplest way.

https://codereview.chromium.org/2898723003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:113: if (SyncService*
service = sync_service_provider_.Run()) {
i think it's fairly uncommon to do assignment in a conditional (outside of
Blink)

can we split this into 2 lines?  i know the old code did this but i never ever
see this in webui code

SyncService* service = sync_service_provider_.Run();
if (!service)
  return;

see also: L155

https://codereview.chromium.org/2898723003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:242: SyncService*
service = sync_service_provider_.Run();
can we get this later (inside both ifs) or early out if it's null?

[commit-bot: I haz the power, 2017-05-23 00:05:45]

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

[commit-bot: I haz the power, 2017-05-23 00:05:45]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[skym, 2017-05-24 00:07:03]

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

[commit-bot: I haz the power, 2017-05-24 00:08:53]

Dry run: CQ is trying da patch.

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

[skym, 2017-05-24 00:20:16]

Updates for dbeam and fixing windows browser test.

https://codereview.chromium.org/2898723003/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/2898723003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:58:
UnregisterModelNotifications();
On 2017/05/22 23:31:43, Dan Beam wrote:
> can we just call weak_ptr_factory_.InvalidateWeakPtrs() here to sever the
> connection to any async, long-living callbacks?  this doesn't really change
much
> about existing code and just ensures what when the page is refreshed or a
> renderer dies, there will be no callbacks called until javascript is allowed
> again.
> 
> if the JS (as it's initialized) always calls the same messages to get
> information, disconnecting the weak pointers for existing calls and then
> re-doing the work when the JS is created seems like the simplest way.

Woah. It seems I can call WeakPtrFactory::Invalidate(), and then start handing
out new weak pointers and the new ones work from that same factory object. I
didn't think it was going to be so easy. Done.

https://codereview.chromium.org/2898723003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:113: if (SyncService*
service = sync_service_provider_.Run()) {
On 2017/05/22 23:31:43, Dan Beam wrote:
> i think it's fairly uncommon to do assignment in a conditional (outside of
> Blink)
> 
> can we split this into 2 lines?  i know the old code did this but i never ever
> see this in webui code
> 
> SyncService* service = sync_service_provider_.Run();
> if (!service)
>   return;
> 
> see also: L155

Moved this invocation out of the if check, added an early return. I didn't
change line 155, not sure if you were asking me to or just referencing where we
don't put it into the if check.

https://codereview.chromium.org/2898723003/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:242: SyncService*
service = sync_service_provider_.Run();
On 2017/05/22 23:31:43, Dan Beam wrote:
> can we get this later (inside both ifs) or early out if it's null?

Done, and removed the DCHECKs that don't make sense anymore.

[commit-bot: I haz the power, 2017-05-24 04:55:33]

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

[commit-bot: I haz the power, 2017-05-24 04:55:34]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Patrick Noland, 2017-05-25 00:13:41]

lgtm

[Dan Beam, 2017-05-25 00:28:21]

lgtm!

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:121: }
nit: no curlies

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:249: if (!service) {
nit: no curlies

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc (right):

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc:166: int
about_sync_data_delegate_call_count() {
nit: const {

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc:170:
SyncService* last_delegate_sync_service() {
const SyncService* last_delegate_sync_service() const {

[skym, 2017-05-25 17:38:12]

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler.cc (right):

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:121: }
On 2017/05/25 00:28:20, Dan Beam wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler.cc:249: if (!service) {
On 2017/05/25 00:28:20, Dan Beam wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc (right):

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc:166: int
about_sync_data_delegate_call_count() {
On 2017/05/25 00:28:20, Dan Beam wrote:
> nit: const {

Done.

https://codereview.chromium.org/2898723003/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/sync_internals_message_handler_unittest.cc:170:
SyncService* last_delegate_sync_service() {
On 2017/05/25 00:28:20, Dan Beam wrote:
> const SyncService* last_delegate_sync_service() const {

Done.

[skym, 2017-05-25 17:38:19]

The CQ bit was checked by skym@chromium.org

[skym, 2017-05-25 17:38:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
afakhry@chromium.org, dbeam@chromium.org, pnoland@chromium.org
Link to the patchset: https://codereview.chromium.org/2898723003/#ps100001
(title: "More updates for dbeam.")

[commit-bot: I haz the power, 2017-05-25 17:38: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, 2017-05-25 19:36:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495733899335220,
"parent_rev": "3040162868fcdfd2d3bf53471a51e764ecfeb18f", "commit_rev":
"b522e5fd22c3454dff23fc48b43bafebd160a543"}

[commit-bot: I haz the power, 2017-05-25 19:36:34]

Description was changed from

==========
[Sync] Migrate SyncInternalsMessageHandler off CallJavascriptFunctionUnsafe.

This CL is a result of comments in
https://codereview.chromium.org/2872023002/ that became too large to be
included in the original code review. These changes should make
SyncInternalsMessageHandler behave more safely in regards to when
javascript methods are invoked, as well as increasing test coverage.

* Replaced CallJavascriptFunctionUnsafe with CallJavascriptFunction.
This mostly worked by adding AllowJavascript to all callbacks from
javascript, and unregistering from model callbacks/observations in
OnJavascriptDisallowed(). However, this didn't work for
OnReceivedAllNodes, which is called asycronously. Instead we need to
manually verify IsJavascriptAllowed() is still true. 
* Moved SigninManagerBase accessor from ProfileSyncService to
SyncService[Base]. This allowed us to remove the SyncManagerBase param
from the ConstructAboutInformation function. In general we want to move
things out of ProfileSyncService that don't have a reason to be there.
* Reworked how SyncInternalsMessageHandler accesses a SyncService to
facilitate unit tests.
* Replaced extractor with callback to be more similar to SyncService
injection pattern.
* Added test coverage for registration/unregistration.

BUG=719044
==========

to

==========
[Sync] Migrate SyncInternalsMessageHandler off CallJavascriptFunctionUnsafe.

This CL is a result of comments in
https://codereview.chromium.org/2872023002/ that became too large to be
included in the original code review. These changes should make
SyncInternalsMessageHandler behave more safely in regards to when
javascript methods are invoked, as well as increasing test coverage.

* Replaced CallJavascriptFunctionUnsafe with CallJavascriptFunction.
This mostly worked by adding AllowJavascript to all callbacks from
javascript, and unregistering from model callbacks/observations in
OnJavascriptDisallowed(). However, this didn't work for
OnReceivedAllNodes, which is called asycronously. Instead we need to
manually verify IsJavascriptAllowed() is still true. 
* Moved SigninManagerBase accessor from ProfileSyncService to
SyncService[Base]. This allowed us to remove the SyncManagerBase param
from the ConstructAboutInformation function. In general we want to move
things out of ProfileSyncService that don't have a reason to be there.
* Reworked how SyncInternalsMessageHandler accesses a SyncService to
facilitate unit tests.
* Replaced extractor with callback to be more similar to SyncService
injection pattern.
* Added test coverage for registration/unregistration.

BUG=719044

Review-Url: https://codereview.chromium.org/2898723003
Cr-Commit-Position: refs/heads/master@{#474755}
Committed:
https://chromium.googlesource.com/chromium/src/+/b522e5fd22c3454dff23fc48b43b...
==========

[commit-bot: I haz the power, 2017-05-25 19:36:36]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b522e5fd22c3454dff23fc48b43b...