Follow-up changes to https://codereview.chromium.org/795773002/

Follow-up changes to https://codereview.chromium.org/795773002/

Per post-commit comments.

BUG=439134
Committed: https://crrev.com/21d28490ec56bff5d435ed29d776450a0adf4a75
Cr-Commit-Position: refs/heads/master@{#314063}

[davidben, 2015-01-23 21:05:47]

davidben@chromium.org changed reviewers:
+ mmenke@chromium.org, pneubeck@chromium.org

[mmenke, 2015-01-23 21:43:27]

Test changes seem reasonable...DeleteSoon makes me shudder.  :(

https://codereview.chromium.org/873723003/diff/40001/content/browser/loader/r...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/873723003/diff/40001/content/browser/loader/r...
content/browser/loader/resource_loader_unittest.cc:49: // SSLCertRequestInfo.
For now it seems that we cannot neither save the
While you're here...."cannot neither" -> "can neither"

[davidben, 2015-01-23 21:50:56]

Yeah, I don't have strong opinions on whether DeleteSoon or documenting that the
callback may cause the object to be deleted is better. I suppose a third option
would be to PostTask when calling the callback.

I'll let you and pneubeck fight over that one I guess. :-)

https://codereview.chromium.org/873723003/diff/40001/content/browser/loader/r...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/873723003/diff/40001/content/browser/loader/r...
content/browser/loader/resource_loader_unittest.cc:49: // SSLCertRequestInfo.
For now it seems that we cannot neither save the
On 2015/01/23 21:43:27, mmenke wrote:
> While you're here...."cannot neither" -> "can neither"

Done.

[mmenke, 2015-01-23 21:57:38]

On 2015/01/23 21:50:56, David Benjamin wrote:
> Yeah, I don't have strong opinions on whether DeleteSoon or documenting that
the
> callback may cause the object to be deleted is better. I suppose a third
option
> would be to PostTask when calling the callback.
> 
> I'll let you and pneubeck fight over that one I guess. :-)

I'm fine with a post task...Actually, we could just remove the method
completely, couldn't we?  Replace the place where we bind it with just
"callback_"

[davidben, 2015-01-23 22:07:59]

On 2015/01/23 21:57:38, mmenke wrote:
> On 2015/01/23 21:50:56, David Benjamin wrote:
> > Yeah, I don't have strong opinions on whether DeleteSoon or documenting that
> the
> > callback may cause the object to be deleted is better. I suppose a third
> option
> > would be to PostTask when calling the callback.
> > 
> > I'll let you and pneubeck fight over that one I guess. :-)
> 
> I'm fine with a post task...Actually, we could just remove the method
> completely, couldn't we?  Replace the place where we bind it with just
> "callback_"

Hrm, actually the PostTask seems silly because we already just posted a task to
do UI -> IO. Pushing the callback back up works for the main call, but not as
easily for the calls in DidGetClientCerts. I suppose either a PostTask or just
pushing the callback back up is saying that the callback is allowed to delete
the SSLClientAuthHandler. It's just a question of how SSLClientAuthHandler
chooses to cope (comment at the bottom, PostTask, or arranging to not be on the
stack).

[pneubeck (no reviews), 2015-01-28 10:31:56]

On 2015/01/23 22:07:59, David Benjamin wrote:
> On 2015/01/23 21:57:38, mmenke wrote:
> > On 2015/01/23 21:50:56, David Benjamin wrote:
> > > Yeah, I don't have strong opinions on whether DeleteSoon or documenting
that
> > the
> > > callback may cause the object to be deleted is better. I suppose a third
> > option
> > > would be to PostTask when calling the callback.
> > > 
> > > I'll let you and pneubeck fight over that one I guess. :-)
> > 
> > I'm fine with a post task...Actually, we could just remove the method
> > completely, couldn't we?  Replace the place where we bind it with just
> > "callback_"
> 
> Hrm, actually the PostTask seems silly because we already just posted a task
to
> do UI -> IO. Pushing the callback back up works for the main call, but not as
> easily for the calls in DidGetClientCerts.
The
  CertificateSelected(NULL);
could be replaced by a PostTask(IO,...).
However, if we remove CertificateSelected and replace it directly by callback_,
then the WeakPtr of SSLClientAuthHandler is not checked anymore and the call
back might happen after SSLClientAuthHandler is already destroyed. That would be
contrary to the attempt to make the lifetime of the handler stricter / more well
defined.

Instead of dropping CertificateSelected, you could make it a static method that
gets the WeakPtr<SSLClientAuthHandler> and the callback_ as arguments and then
always PostTask.

That appears to be a tad more safe as it doesn't depend on the callback behaving
correctly, but the current patches also lgtm.

> I suppose either a PostTask or just
> pushing the callback back up is saying that the callback is allowed to delete
> the SSLClientAuthHandler. It's just a question of how SSLClientAuthHandler
> chooses to cope (comment at the bottom, PostTask, or arranging to not be on
the
> stack).

[mmenke, 2015-01-30 20:31:17]

On 2015/01/28 10:31:56, pneubeck wrote:
> On 2015/01/23 22:07:59, David Benjamin wrote:
> > On 2015/01/23 21:57:38, mmenke wrote:
> > > On 2015/01/23 21:50:56, David Benjamin wrote:
> > > > Yeah, I don't have strong opinions on whether DeleteSoon or documenting
> that
> > > the
> > > > callback may cause the object to be deleted is better. I suppose a third
> > > option
> > > > would be to PostTask when calling the callback.
> > > > 
> > > > I'll let you and pneubeck fight over that one I guess. :-)
> > > 
> > > I'm fine with a post task...Actually, we could just remove the method
> > > completely, couldn't we?  Replace the place where we bind it with just
> > > "callback_"
> > 
> > Hrm, actually the PostTask seems silly because we already just posted a task
> to
> > do UI -> IO. Pushing the callback back up works for the main call, but not
as
> > easily for the calls in DidGetClientCerts.
> The
>   CertificateSelected(NULL);
> could be replaced by a PostTask(IO,...).
> However, if we remove CertificateSelected and replace it directly by
callback_,
> then the WeakPtr of SSLClientAuthHandler is not checked anymore and the call
> back might happen after SSLClientAuthHandler is already destroyed. That would
be
> contrary to the attempt to make the lifetime of the handler stricter / more
well
> defined.
> 
> Instead of dropping CertificateSelected, you could make it a static method
that
> gets the WeakPtr<SSLClientAuthHandler> and the callback_ as arguments and then
> always PostTask.
> 
> That appears to be a tad more safe as it doesn't depend on the callback
behaving
> correctly, but the current patches also lgtm.

I'm fine with that...  Most anything that gets rid of the DeleteSoon I can live
with.  David:  Are you planning on updating this CL, or can I remove myself as a
reviewer?  I'm having trouble keeping track of the 15 CLs nominally in my review
queue, all updated in the past week.

[davidben, 2015-01-30 20:34:15]

On 2015/01/30 20:31:17, mmenke wrote:
> I'm fine with that...  Most anything that gets rid of the DeleteSoon I can
live
> with.  David:  Are you planning on updating this CL, or can I remove myself as
a
> reviewer?  I'm having trouble keeping track of the 15 CLs nominally in my
review
> queue, all updated in the past week.

Oh, yeah, sorry, I'll update this today. Been doing the crash fix on top of it,
but it would be good to clear this one. :-)

[davidben, 2015-01-30 21:11:42]

On 2015/01/28 10:31:56, pneubeck wrote:
> On 2015/01/23 22:07:59, David Benjamin wrote:
> > On 2015/01/23 21:57:38, mmenke wrote:
> > > On 2015/01/23 21:50:56, David Benjamin wrote:
> > > > Yeah, I don't have strong opinions on whether DeleteSoon or documenting
> that
> > > the
> > > > callback may cause the object to be deleted is better. I suppose a third
> > > option
> > > > would be to PostTask when calling the callback.
> > > > 
> > > > I'll let you and pneubeck fight over that one I guess. :-)
> > > 
> > > I'm fine with a post task...Actually, we could just remove the method
> > > completely, couldn't we?  Replace the place where we bind it with just
> > > "callback_"
> > 
> > Hrm, actually the PostTask seems silly because we already just posted a task
> to
> > do UI -> IO. Pushing the callback back up works for the main call, but not
as
> > easily for the calls in DidGetClientCerts.
> The
>   CertificateSelected(NULL);
> could be replaced by a PostTask(IO,...).
> However, if we remove CertificateSelected and replace it directly by
callback_,
> then the WeakPtr of SSLClientAuthHandler is not checked anymore and the call
> back might happen after SSLClientAuthHandler is already destroyed. That would
be
> contrary to the attempt to make the lifetime of the handler stricter / more
well
> defined.
> 
> Instead of dropping CertificateSelected, you could make it a static method
that
> gets the WeakPtr<SSLClientAuthHandler> and the callback_ as arguments and then
> always PostTask.
> 
> That appears to be a tad more safe as it doesn't depend on the callback
behaving
> correctly, but the current patches also lgtm.

I'm not sure I see how that's appreciably different from PostTasking callback_
directly. I've gone ahead and done that.

Note that all this is basically a distraction because I'm going to be changing
this immediately afterwards anyway. (We need separate "cancel" and "continue
with no certificate" hooks.)

[davidben, 2015-01-30 21:45:39]

On 2015/01/30 21:11:42, David Benjamin wrote:
> I'm not sure I see how that's appreciably different from PostTasking callback_
> directly. I've gone ahead and done that.
> 
> Note that all this is basically a distraction because I'm going to be changing
> this immediately afterwards anyway. (We need separate "cancel" and "continue
> with no certificate" hooks.)

Hrm, no, there is a difference: destroying the handler should guarantee the
callback doesn't get called anymore. I've just undone all those changes for now
so we can land the other bits. I'll try to come up with something that'll
appease everyone's tastes in the new one, but since I'm going to change this to
a Delegate anyway, it's just unnecessary churn to solve this twice.

[mmenke, 2015-01-30 22:05:24]

On 2015/01/30 21:45:39, David Benjamin wrote:
> On 2015/01/30 21:11:42, David Benjamin wrote:
> > I'm not sure I see how that's appreciably different from PostTasking
callback_
> > directly. I've gone ahead and done that.
> > 
> > Note that all this is basically a distraction because I'm going to be
changing
> > this immediately afterwards anyway. (We need separate "cancel" and "continue
> > with no certificate" hooks.)
> 
> Hrm, no, there is a difference: destroying the handler should guarantee the
> callback doesn't get called anymore. I've just undone all those changes for
now
> so we can land the other bits. I'll try to come up with something that'll
> appease everyone's tastes in the new one, but since I'm going to change this
to
> a Delegate anyway, it's just unnecessary churn to solve this twice.

LGTM

[pneubeck (no reviews), 2015-01-31 10:34:29]

i'm loosing overview over the reasoning why some of the proposals are not
working.
since you're planning more changes anyways, that's probably irrelevant anyways.

lgtm

[davidben, 2015-01-31 17:34:57]

I still need to write tests for it and stuff, but this is what the further
changes will possibly look like, by the way:
https://codereview.chromium.org/859213006/

I did the static method + WeakPtr thing.

[davidben, 2015-01-31 17:36:26]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-01-31 17:37:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/873723003/140001

[commit-bot: I haz the power, 2015-01-31 18:35:08]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-01-31 18:37:07]

Patchset 8 (id:??) landed as
https://crrev.com/21d28490ec56bff5d435ed29d776450a0adf4a75
Cr-Commit-Position: refs/heads/master@{#314063}