Fix use-after-free in HDDDTC shutdown

Fix use-after-free in HDDDTC shutdown

We were trying to unregister the HistoryDeleteDirective DTC at shutdown, but
the DTC shutdown happens as part of the ProfileSyncService shutdown, and is
destroyed after the observers_ object is destroyed. As such, there's no need
to remove the observer.

This patch also unreverts 6513171d7473b9eb60e1bf16369cf893daa33a07
Original codereview at https://codereview.chromium.org/534733002

BUG=409965
368834

Committed: https://crrev.com/40dbafab4cfef214d7a4bae22c8d698936ed6f5f
Cr-Commit-Position: refs/heads/master@{#293824}

[Nicolas Zea, 2014-09-02 22:49:45]

zea@chromium.org changed reviewers:
+ maniscalco@chromium.org

[Nicolas Zea, 2014-09-02 22:50:03]

+Nick, PTAL. The first patchset contains the diff from the original patch.

[maniscalco, 2014-09-02 23:26:23]

https://codereview.chromium.org/535683002/diff/1/chrome/browser/sync/glue/his...
File chrome/browser/sync/glue/history_delete_directives_data_type_controller.cc
(right):

https://codereview.chromium.org/535683002/diff/1/chrome/browser/sync/glue/his...
chrome/browser/sync/glue/history_delete_directives_data_type_controller.cc:18:
ProfileSyncService* sync_service)
So the problem was that sync_service_ was destroyed before
HistoryDeleteDirectivesDataTypeController tried to access it in its dtor.

Is there any signal HDDDTC can use to know that sync_service_ is no longer a
valid pointer (or will be soon)?  How about holding a
WeakPtr<ProfileSyncService> instead of a ProfileSyncService* ?

[Nicolas Zea, 2014-09-04 16:35:06]

PTAL

https://codereview.chromium.org/535683002/diff/1/chrome/browser/sync/glue/his...
File chrome/browser/sync/glue/history_delete_directives_data_type_controller.cc
(right):

https://codereview.chromium.org/535683002/diff/1/chrome/browser/sync/glue/his...
chrome/browser/sync/glue/history_delete_directives_data_type_controller.cc:18:
ProfileSyncService* sync_service)
On 2014/09/02 23:26:23, maniscalco wrote:
> So the problem was that sync_service_ was destroyed before
> HistoryDeleteDirectivesDataTypeController tried to access it in its dtor.
> 
> Is there any signal HDDDTC can use to know that sync_service_ is no longer a
> valid pointer (or will be soon)?  How about holding a
> WeakPtr<ProfileSyncService> instead of a ProfileSyncService* ?

Yeah, this kind of built in assumption is kindda hacky. I think the real issue
is that the observer should be destroyed last (and created first) in the PSS.
Went ahead and changed that, so that other members can add/remove themselves as
observers without issue.

[Nicolas Zea, 2014-09-05 00:02:18]

Patchset #4 (id:60001) has been deleted

[Nicolas Zea, 2014-09-05 00:08:45]

PTAL. LoadModels and StopModels are always called before the type is started and
before the type is destroyed, so I think this should fix things. Filed 411092
for the weak ptr factory issue.

[maniscalco, 2014-09-05 15:51:24]

lgtm

[Nicolas Zea, 2014-09-05 17:03:37]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-09-05 17:05:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/535683002/80001

[commit-bot: I haz the power, 2014-09-05 17:21:53]

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

[commit-bot: I haz the power, 2014-09-05 17:21:55]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nicolas Zea, 2014-09-05 17:43:50]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-09-05 17:44:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/535683002/100001

[commit-bot: I haz the power, 2014-09-05 19:09:24]

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

[commit-bot: I haz the power, 2014-09-05 19:09:25]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nicolas Zea, 2014-09-08 19:00:14]

Could you take a look at the last patchset? Turns out the integration tests
weren't passing because I was overriding the wrong method (and was improperly
using ShouldPushChanges). The fix wasn't really trivial, so worth another look.

[maniscalco, 2014-09-08 21:57:55]

lgtm

[Nicolas Zea, 2014-09-08 23:38:44]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-09-08 23:39:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/535683002/120001

[commit-bot: I haz the power, 2014-09-09 00:55:37]

Committed patchset #6 (id:120001) as dcd1016a2f07bfc1c4e918ed883a4e787b88107d

[commit-bot: I haz the power, 2014-09-10 03:49:22]

Patchset 6 (id:??) landed as
https://crrev.com/40dbafab4cfef214d7a4bae22c8d698936ed6f5f
Cr-Commit-Position: refs/heads/master@{#293824}