Make GlobalErrorService's ownership model slightly less insane.

Make GlobalErrorService's ownership model slightly less insane.

GlobalErrorService ownership was crazy; sometimes the error objects contained were owned, and sometimes they weren't. This separates the two cases into clear statements of ownership, and deprecates the case where the error objects are unowned.

BUG=555865, 673578
Committed: https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed
Cr-Commit-Position: refs/heads/master@{#438192}

[Avi (use Gerrit), 2016-10-08 22:28:24]

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

[commit-bot: I haz the power, 2016-10-08 22:28:27]

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

[Avi (use Gerrit), 2016-10-08 22:32:02]

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

[commit-bot: I haz the power, 2016-10-08 22:32:10]

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

[Avi (use Gerrit), 2016-10-08 22:39:16]

avi@chromium.org changed reviewers:
+ thakis@chromium.org

[Avi (use Gerrit), 2016-10-08 22:39:17]

At least this change makes it clear that the ownership is crazy...

Note that before, we would often have two owners of objects, but relying on a
specific destruction order to make sure that one owner relinquished ownership
first.

And of course, everyone else just leaked things all over the place.

[commit-bot: I haz the power, 2016-10-08 23:03:36]

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

[commit-bot: I haz the power, 2016-10-08 23:03:37]

Dry run: Exceeded global retry quota

[Avi (use Gerrit), 2016-10-09 18:02:21]

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

[commit-bot: I haz the power, 2016-10-09 18:02:32]

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-10-09 18:45:08]

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

[commit-bot: I haz the power, 2016-10-09 18:45:09]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-09 18:57:44]

ok, this now passes all trybots. ptal.

[Nico, 2016-10-10 14:03:37]

What a mess. It does look like it should be doable to make all calls to
AddUnownedGlobalError() go away though, so I think we should try to make
AddGlobalError() always assume ownership

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_disabled_ui.cc:434:
->RemoveOwnedGlobalError(this).release();
Hm, I think this'd be nicer if it stored the result in a local and then called
release right next to the call to DeleteSoon().

DeleteSoon() should probably have a version taking a unique_ptr, then the return
value could be moved to DeleteSoon().

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/external_install_error.cc (right):

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/external_install_error.cc:441: global_error_.reset(new
ExternalInstallBubbleAlert(this, prompt_.get()));
This one looks like it could be a weak ptr and this class could use the
AddOwned() version throughout

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/recovery...
File chrome/browser/recovery/recovery_install_global_error.cc (right):

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/recovery...
chrome/browser/recovery/recovery_install_global_error.cc:27: this);
this ctor has only one call, so making this use the owned variant should be
possible as well
(https://cs.chromium.org/chromium/src/chrome/browser/recovery/recovery_install...)

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_global_error.cc (right):

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/sync/syn...
chrome/browser/sync/sync_global_error.cc:33:
global_error_service_->AddUnownedGlobalError(this);
likewise:
https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_global_error_fa...

[Avi (use Gerrit), 2016-10-10 16:52:36]

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_disabled_ui.cc:434:
->RemoveOwnedGlobalError(this).release();
On 2016/10/10 14:03:37, Nico wrote:
> Hm, I think this'd be nicer if it stored the result in a local and then called
> release right next to the call to DeleteSoon().
> 
> DeleteSoon() should probably have a version taking a unique_ptr, then the
return
> value could be moved to DeleteSoon().

Done.

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/external_install_error.cc (right):

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/external_install_error.cc:441: global_error_.reset(new
ExternalInstallBubbleAlert(this, prompt_.get()));
On 2016/10/10 14:03:37, Nico wrote:
> This one looks like it could be a weak ptr and this class could use the
> AddOwned() version throughout

The GlobalErrorService is a PKS, and is guaranteed to outlive this, so I'm
making it a weak pointer.

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/recovery...
File chrome/browser/recovery/recovery_install_global_error.cc (right):

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/recovery...
chrome/browser/recovery/recovery_install_global_error.cc:27: this);
On 2016/10/10 14:03:37, Nico wrote:
> this ctor has only one call, so making this use the owned variant should be
> possible as well
>
(https://cs.chromium.org/chromium/src/chrome/browser/recovery/recovery_install...)

Done.

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_global_error.cc (right):

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/sync/syn...
chrome/browser/sync/sync_global_error.cc:33:
global_error_service_->AddUnownedGlobalError(this);
On 2016/10/10 14:03:37, Nico wrote:
> likewise:
>
https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_global_error_fa...

This is a profile-keyed service and is rather hard to kill. I'm going to have to
get back to you on this.

[Avi (use Gerrit), 2016-10-16 22:38:10]

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

[commit-bot: I haz the power, 2016-10-16 22:38:25]

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-10-16 23:16:13]

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

[commit-bot: I haz the power, 2016-10-16 23:16:14]

Dry run: Exceeded global retry quota

[Avi (use Gerrit), 2016-10-16 23:24:19]

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

[commit-bot: I haz the power, 2016-10-16 23:24:27]

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-10-17 00:07:06]

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

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

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-10-17 00:19:28]

Stuff keeps breaking the more clever I get. This is about as good as it gets
without going over the line.

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/recovery...
File chrome/browser/recovery/recovery_install_global_error.cc (right):

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/recovery...
chrome/browser/recovery/recovery_install_global_error.cc:27: this);
On 2016/10/10 16:52:36, Avi wrote:
> On 2016/10/10 14:03:37, Nico wrote:
> > this ctor has only one call, so making this use the owned variant should be
> > possible as well
> >
>
(https://cs.chromium.org/chromium/src/chrome/browser/recovery/recovery_install...)
> 
> Done.

This got past what I was comfortable doing, so no.

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/sync/syn...
File chrome/browser/sync/sync_global_error.cc (right):

https://codereview.chromium.org/2409443002/diff/40001/chrome/browser/sync/syn...
chrome/browser/sync/sync_global_error.cc:33:
global_error_service_->AddUnownedGlobalError(this);
On 2016/10/10 16:52:36, Avi wrote:
> On 2016/10/10 14:03:37, Nico wrote:
> > likewise:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_global_error_fa...
> 
> This is a profile-keyed service and is rather hard to kill. I'm going to have
to
> get back to you on this.

Oh, boy, no.

This is a PKS. Lots of moving pieces depend on it, and I very much do not feel
comfortable fixing its broken ownership issues in this patch.

[Avi (use Gerrit), 2016-12-12 18:41:14]

Nico—

I want to try to get back to this.

I tried to do what you said, to only have owned errors. The problem is that lots
of the code generates owned errors with PKSs, and I am not familiar enough with
the code to disentangle them and make them unowned so that the
GlobalErrorService can own everything. I tried, and it got worse and worse.

This is a cleaned equivalent to what we have today.

Please review.

[Nico, 2016-12-12 22:32:51]

Alright; thanks for trying. One small request:

https://codereview.chromium.org/2409443002/diff/80001/chrome/browser/ui/globa...
File chrome/browser/ui/global_error/global_error_service.h (right):

https://codereview.chromium.org/2409443002/diff/80001/chrome/browser/ui/globa...
chrome/browser/ui/global_error/global_error_service.h:41: void
AddUnownedGlobalError(GlobalError* error);
Can we make this the default version (and call it AddGlobalError) and give the
Unowned version a "Do not use" comment (and file a bug for fixing this
eventually)?

[Avi (use Gerrit), 2016-12-13 01:18:54]

Description was changed from

==========
Make GlobalErrorService's ownership model slightly less insane.

BUG=555865
==========

to

==========
Make GlobalErrorService's ownership model slightly less insane.

BUG=555865,673578
==========

[Avi (use Gerrit), 2016-12-13 01:23:17]

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

[commit-bot: I haz the power, 2016-12-13 01:25:00]

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-13 03:03:30]

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

[commit-bot: I haz the power, 2016-12-13 03:03:31]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-12-13 03:06:19]

https://codereview.chromium.org/2409443002/diff/80001/chrome/browser/ui/globa...
File chrome/browser/ui/global_error/global_error_service.h (right):

https://codereview.chromium.org/2409443002/diff/80001/chrome/browser/ui/globa...
chrome/browser/ui/global_error/global_error_service.h:41: void
AddUnownedGlobalError(GlobalError* error);
On 2016/12/12 22:32:50, Nico wrote:
> Can we make this the default version (and call it AddGlobalError) and give the
> Unowned version a "Do not use" comment (and file a bug for fixing this
> eventually)?

Done.

[Nico, 2016-12-13 15:54:14]

lgtm; maybe you can make the cl description a bit more detailed

https://codereview.chromium.org/2409443002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/srt_global_error_win.cc (right):

https://codereview.chromium.org/2409443002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/srt_global_error_win.cc:266: global_error_service_
= nullptr;
isn't `this` dead after the release() call in the previous line?

https://codereview.chromium.org/2409443002/diff/100001/chrome/browser/ui/glob...
File chrome/browser/ui/global_error/global_error_service_browsertest.cc (right):

https://codereview.chromium.org/2409443002/diff/100001/chrome/browser/ui/glob...
chrome/browser/ui/global_error/global_error_service_browsertest.cc:125:
std::unique_ptr<BubbleViewError> error(new BubbleViewError);
Does this one have to be unowned? Or did you leave this one unowned to have some
coverage for that path?

[Avi (use Gerrit), 2016-12-13 15:59:57]

Description was changed from

==========
Make GlobalErrorService's ownership model slightly less insane.

BUG=555865,673578
==========

to

==========
Make GlobalErrorService's ownership model slightly less insane.

GlobalErrorService ownership was crazy; sometimes the error objects contained
were owned, and sometimes they weren't. This separates the two cases into clear
statements of ownership, and deprecates the case where the error objects are
unowned.

BUG=555865,673578
==========

[Avi (use Gerrit), 2016-12-13 16:06:00]

https://codereview.chromium.org/2409443002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/srt_global_error_win.cc (right):

https://codereview.chromium.org/2409443002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/srt_global_error_win.cc:266: global_error_service_
= nullptr;
On 2016/12/13 15:54:14, Nico wrote:
> isn't `this` dead after the release() call in the previous line?

No; this is a release(), not a reset(). It's taking ownership back from the
global error service.

https://codereview.chromium.org/2409443002/diff/100001/chrome/browser/ui/glob...
File chrome/browser/ui/global_error/global_error_service_browsertest.cc (right):

https://codereview.chromium.org/2409443002/diff/100001/chrome/browser/ui/glob...
chrome/browser/ui/global_error/global_error_service_browsertest.cc:125:
std::unique_ptr<BubbleViewError> error(new BubbleViewError);
On 2016/12/13 15:54:14, Nico wrote:
> Does this one have to be unowned? Or did you leave this one unowned to have
some
> coverage for that path?

I'll pretend this was the latter :) Adding a comment.

[Avi (use Gerrit), 2016-12-13 16:06:56]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2016-12-13 16:06:57]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2409443002/#ps120001
(title: "commentary")

[commit-bot: I haz the power, 2016-12-13 16:07:20]

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-13 16:55:23]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481645216675870,
"parent_rev": "b8854e7fa3d2d1530cb59a327ce8188e29d2a89e", "commit_rev":
"19eeef8a10fcea6f1fe6d427d9ed5d88c9798d97"}

[commit-bot: I haz the power, 2016-12-13 16:55:49]

Description was changed from

==========
Make GlobalErrorService's ownership model slightly less insane.

GlobalErrorService ownership was crazy; sometimes the error objects contained
were owned, and sometimes they weren't. This separates the two cases into clear
statements of ownership, and deprecates the case where the error objects are
unowned.

BUG=555865,673578
==========

to

==========
Make GlobalErrorService's ownership model slightly less insane.

GlobalErrorService ownership was crazy; sometimes the error objects contained
were owned, and sometimes they weren't. This separates the two cases into clear
statements of ownership, and deprecates the case where the error objects are
unowned.

BUG=555865,673578

Review-Url: https://codereview.chromium.org/2409443002
==========

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

Committed patchset #7 (id:120001)

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

Description was changed from

==========
Make GlobalErrorService's ownership model slightly less insane.

GlobalErrorService ownership was crazy; sometimes the error objects contained
were owned, and sometimes they weren't. This separates the two cases into clear
statements of ownership, and deprecates the case where the error objects are
unowned.

BUG=555865,673578

Review-Url: https://codereview.chromium.org/2409443002
==========

to

==========
Make GlobalErrorService's ownership model slightly less insane.

GlobalErrorService ownership was crazy; sometimes the error objects contained
were owned, and sometimes they weren't. This separates the two cases into clear
statements of ownership, and deprecates the case where the error objects are
unowned.

BUG=555865,673578

Committed: https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed
Cr-Commit-Position: refs/heads/master@{#438192}
==========

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

Patchset 7 (id:??) landed as
https://crrev.com/2451b25ae9188cfe5e0177c9c306ced7c3a173ed
Cr-Commit-Position: refs/heads/master@{#438192}