No longer keep a lingering reference to TypedUrlDataTypeController after PSS exits

No longer keep a lingering reference to TypedUrlDataTypeController after PSS exits


BUG=127706
TEST=Start chrome, sign in to sync, start print job spooling and exit while the print job is spooling, should not get a crash.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137628

[Andrew T Wilson (Slow), 2012-05-12 00:00:29]

PTAL

[lipalani1, 2012-05-14 22:46:17]

I am mostly OK with this change and feel comfortable L G T M ing it as such.

let me know what you think of the following comments.

http://codereview.chromium.org/10391080/diff/8002/chrome/browser/sync/glue/no...
File chrome/browser/sync/glue/non_frontend_data_type_controller.cc (right):

http://codereview.chromium.org/10391080/diff/8002/chrome/browser/sync/glue/no...
chrome/browser/sync/glue/non_frontend_data_type_controller.cc:476:
&NonFrontendDataTypeController::StopAssociation, this))) {
Would it make sense to have a comment here saying something to the effect of
"when this task is executed we expect the reference to the task should be
removed towards the end. So the fact that the next task is executed would mean
we have no lingering references".

http://codereview.chromium.org/10391080/diff/8002/chrome/browser/sync/glue/ty...
File chrome/browser/sync/glue/typed_url_data_type_controller.cc (right):

http://codereview.chromium.org/10391080/diff/8002/chrome/browser/sync/glue/ty...
chrome/browser/sync/glue/typed_url_data_type_controller.cc:25: 
Nit - if possible we should unify these 2 classes.

http://codereview.chromium.org/10391080/diff/8002/chrome/browser/sync/glue/ty...
chrome/browser/sync/glue/typed_url_data_type_controller.cc:65: return true;
It might make sense to delete the task here. That way if we were shutdown before
starting we would not hit the crash.

http://codereview.chromium.org/10391080/diff/8002/chrome/browser/sync/glue/ty...
chrome/browser/sync/glue/typed_url_data_type_controller.cc:173:
base::Bind(&TypedUrlDataTypeController::SetBackend, this)),
If we unified the class this block of code might not be needed.

[Andrew T Wilson (Slow), 2012-05-15 22:52:08]

PTAL

https://chromiumcodereview.appspot.com/10391080/diff/8002/chrome/browser/sync...
File chrome/browser/sync/glue/non_frontend_data_type_controller.cc (right):

https://chromiumcodereview.appspot.com/10391080/diff/8002/chrome/browser/sync...
chrome/browser/sync/glue/non_frontend_data_type_controller.cc:476:
&NonFrontendDataTypeController::StopAssociation, this))) {
On 2012/05/14 22:46:17, lipalani1 wrote:
> Would it make sense to have a comment here saying something to the effect of
> "when this task is executed we expect the reference to the task should be
> removed towards the end. So the fact that the next task is executed would mean
> we have no lingering references".
I clarified the existing comment.

https://chromiumcodereview.appspot.com/10391080/diff/8002/chrome/browser/sync...
File chrome/browser/sync/glue/typed_url_data_type_controller.cc (right):

https://chromiumcodereview.appspot.com/10391080/diff/8002/chrome/browser/sync...
chrome/browser/sync/glue/typed_url_data_type_controller.cc:25: 
On 2012/05/14 22:46:17, lipalani1 wrote:
> Nit - if possible we should unify these 2 classes.
Done.

https://chromiumcodereview.appspot.com/10391080/diff/8002/chrome/browser/sync...
chrome/browser/sync/glue/typed_url_data_type_controller.cc:65: return true;
On 2012/05/14 22:46:17, lipalani1 wrote:
> It might make sense to delete the task here. That way if we were shutdown
before
> starting we would not hit the crash.
Agreed - see my change to RunTaskOnHistoryThread.

https://chromiumcodereview.appspot.com/10391080/diff/8002/chrome/browser/sync...
chrome/browser/sync/glue/typed_url_data_type_controller.cc:173:
base::Bind(&TypedUrlDataTypeController::SetBackend, this)),
On 2012/05/14 22:46:17, lipalani1 wrote:
> If we unified the class this block of code might not be needed.
Done.

[lipalani1, 2012-05-15 22:59:30]

LGTM

[commit-bot: I haz the power, 2012-05-15 23:06:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/10391080/7003

[commit-bot: I haz the power, 2012-05-15 23:44:53]

Try job failure for 10391080-7003 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Nicolas Zea, 2012-05-16 22:40:57]

http://codereview.chromium.org/10391080/diff/12002/chrome/browser/sync/glue/t...
File chrome/browser/sync/glue/typed_url_data_type_controller.h (right):

http://codereview.chromium.org/10391080/diff/12002/chrome/browser/sync/glue/t...
chrome/browser/sync/glue/typed_url_data_type_controller.h:65:
PrefChangeRegistrar pref_registrar_;
To some extent, I kind of feel like it's a bug for PrefChangeRegistrar to be
using raw pointers instead of WeakPointers in the first place, which I believe
was where the crash came from.

http://codereview.chromium.org/10391080/diff/12002/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10391080/diff/12002/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:164:
DCHECK(iter->second->HasOneRef())
This does not hold true for non-UI thread datatypes implementing the new API,
where not blocking shutdown was one of our goals. In those cases, we don't
bother to wait for them to finish stopping, since we have an disconnect-able
interface that they must go through to interact with sync.

In other words, once DTC::Stop() is called, the datatype won't be able to
interact with the PSS or anything within sync, letting us continue without
blocking shutdown. The DTC ma have running/pending tasks on the datatype's
thread that don't execute until after Stop(), but we ensure correctness.

What do you think about, at least for new DTC's, just making it clear that no
member of the DTC should rely on the profile being around after Stop is called
(either by clearing these members during Stop() or by ensuring they are POD's)?

[commit-bot: I haz the power, 2012-05-17 02:27:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/10391080/8006

[commit-bot: I haz the power, 2012-05-17 04:18:07]

Change committed as 137628