Fix PreferenceConnectionManagerTeardown

Fix PreferenceConnectionManagerTeardown

It is possible for the teardown order of chrome to not be as expected.

PreferenceConnectionManager was expecting to live until profile destruction,
at which point it would close mojo connections.

However it is possible for the connection manager to be deleted early, Leaving
connections open. They would eventually close during shutdown, and attempt to
notify the now dead connection manager.

This change updates PreferenceConnectionManager to use the new StrongBindingSet.
This set handles removing of StrongBindings from the collection when connection
errors occur. Thus replacing the manual handling written in the connection
manager which had error prone base::Unretained(this). The connections are all
deleted when the PreferenceConnectionManager is destroyed.

TEST=manual testing with asan builds
BUG=687933
Review-Url: https://codereview.chromium.org/2710733002
Cr-Commit-Position: refs/heads/master@{#452044}
Committed: https://chromium.googlesource.com/chromium/src/+/0932b390e4b08af8590e8553a7b2a1fc8ef9c99b

[jonross, 2017-02-21 15:55:54]

https://codereview.chromium.org/2710733002/diff/1/chrome/browser/prefs/prefer...
File chrome/browser/prefs/preferences_connection_manager.cc (left):

https://codereview.chromium.org/2710733002/diff/1/chrome/browser/prefs/prefer...
chrome/browser/prefs/preferences_connection_manager.cc:109: }
The functionality that was here was moved to Create (line 94-100 above)

So its no longer needed

[jonross, 2017-02-21 15:56:27]

jonross@chromium.org changed reviewers:
+ sky@chromium.org

[jonross, 2017-02-21 15:56:28]

Hey sky@,

Could you take a look at this change to the shutdown order of preferences
connections.

Thanks,
Jon

[sky, 2017-02-21 18:32:16]

LGTM - is it possible to add test coverage?

[jonross, 2017-02-21 18:42:34]

On 2017/02/21 18:32:16, sky wrote:
> LGTM - is it possible to add test coverage?

Not without making test helpers for StrongBinding and StrongBindingSet. Neither
expose access to the internal bindings, nor a WeakPtr. Which I would need to
verify proper destruction order in a test of PreferencesConnectionManager.

Though the underlying BindingSetTest does provide coverage for connection error
handling, which was the original source of the use-after-free.

[sky, 2017-02-21 21:31:20]

Ok, fair enough.

On Tue, Feb 21, 2017 at 10:42 AM,  <jonross@chromium.org> wrote:
> On 2017/02/21 18:32:16, sky wrote:
>> LGTM - is it possible to add test coverage?
>
> Not without making test helpers for StrongBinding and StrongBindingSet.
> Neither
> expose access to the internal bindings, nor a WeakPtr. Which I would need to
> verify proper destruction order in a test of PreferencesConnectionManager.
>
> Though the underlying BindingSetTest does provide coverage for connection
> error
> handling, which was the original source of the use-after-free.
>
> https://codereview.chromium.org/2710733002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jonross, 2017-02-21 21:31:33]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2017-02-21 21:34:25]

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-02-21 23:39:06]

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

[commit-bot: I haz the power, 2017-02-21 23:39:07]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[jonross, 2017-02-21 23:55:46]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2017-02-21 23:56: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, 2017-02-22 01:59:55]

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

[commit-bot: I haz the power, 2017-02-22 01:59:56]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[jonross, 2017-02-22 14:46:35]

The CQ bit was checked by jonross@chromium.org

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

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-02-22 14:49:59]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487774795830360, "parent_rev":
"2b02351f4bd399714415d786e8b931f2bb14099d", "commit_rev":
"0932b390e4b08af8590e8553a7b2a1fc8ef9c99b"}

[commit-bot: I haz the power, 2017-02-22 14:50:43]

Description was changed from

==========
Fix PreferenceConnectionManagerTeardown

It is possible for the teardown order of chrome to not be as expected.

PreferenceConnectionManager was expecting to live until profile destruction,
at which point it would close mojo connections.

However it is possible for the connection manager to be deleted early, Leaving
connections open. They would eventually close during shutdown, and attempt to
notify the now dead connection manager.

This change updates PreferenceConnectionManager to use the new StrongBindingSet.
This set handles removing of StrongBindings from the collection when connection
errors occur. Thus replacing the manual handling written in the connection
manager which had error prone base::Unretained(this). The connections are all
deleted when the PreferenceConnectionManager is destroyed.

TEST=manual testing with asan builds
BUG=687933
==========

to

==========
Fix PreferenceConnectionManagerTeardown

It is possible for the teardown order of chrome to not be as expected.

PreferenceConnectionManager was expecting to live until profile destruction,
at which point it would close mojo connections.

However it is possible for the connection manager to be deleted early, Leaving
connections open. They would eventually close during shutdown, and attempt to
notify the now dead connection manager.

This change updates PreferenceConnectionManager to use the new StrongBindingSet.
This set handles removing of StrongBindings from the collection when connection
errors occur. Thus replacing the manual handling written in the connection
manager which had error prone base::Unretained(this). The connections are all
deleted when the PreferenceConnectionManager is destroyed.

TEST=manual testing with asan builds
BUG=687933

Review-Url: https://codereview.chromium.org/2710733002
Cr-Commit-Position: refs/heads/master@{#452044}
Committed:
https://chromium.googlesource.com/chromium/src/+/0932b390e4b08af8590e8553a7b2...
==========

[commit-bot: I haz the power, 2017-02-22 14:50:44]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/0932b390e4b08af8590e8553a7b2...