Checks to enforce relative lifetimes for SdchManager.

Checks to enforce relative lifetimes for SdchManager.

BUG=454198
R=mmenke@chromium.org

Committed: https://crrev.com/4733facd42dbafe77042ec65e8d6f63fb0e2977a
Cr-Commit-Position: refs/heads/master@{#320164}

[Randy Smith (Not in Mondays), 2015-03-11 17:28:33]

Matt: I *am* getting what look to be real try job failures in this test, which
I'm excited about, but I'd like you to do a quick review anyway.  No need to
scan for nits or similar--I don't currently plan to land this--but if you could
just verify that you think CHECK failures are indeed likely to be bugs/things
wrong with the code, I would be very grateful.  I'm fairly sure, but I'd like to
be very sure.

[mmenke, 2015-03-11 17:43:10]

On 2015/03/11 17:28:33, rdsmith wrote:
> Matt: I *am* getting what look to be real try job failures in this test, which
> I'm excited about, but I'd like you to do a quick review anyway.  No need to
> scan for nits or similar--I don't currently plan to land this--but if you
could
> just verify that you think CHECK failures are indeed likely to be bugs/things
> wrong with the code, I would be very grateful.  I'm fairly sure, but I'd like
to
> be very sure.

Hrm...The CL isn't quite running the check I had in mind (Which is fine), but
I'm having trouble figuring out how it's causing a crash in tests that don't
exercise SDCH.  In url_request_http_job.cc, if the SDCH manager is deleted, it
should be NULL, so we never call into it.

[Randy Smith (Not in Mondays), 2015-03-11 17:47:38]

On 2015/03/11 17:43:10, mmenke wrote:
> On 2015/03/11 17:28:33, rdsmith wrote:
> > Matt: I *am* getting what look to be real try job failures in this test,
which
> > I'm excited about, but I'd like you to do a quick review anyway.  No need to
> > scan for nits or similar--I don't currently plan to land this--but if you
> could
> > just verify that you think CHECK failures are indeed likely to be
bugs/things
> > wrong with the code, I would be very grateful.  I'm fairly sure, but I'd
like
> to
> > be very sure.
> 
> Hrm...The CL isn't quite running the check I had in mind (Which is fine), but
> I'm having trouble figuring out how it's causing a crash in tests that don't
> exercise SDCH.  In url_request_http_job.cc, if the SDCH manager is deleted, it
> should be NULL, so we never call into it.

So a) What check did you have in mind?  I tried to implement the spirit of
everything I remember you saying, so if there's something missing, I'd like to
look into it.  b) I had thought the narrowing of checks from the previous CL was
to focus just on those areas where SDCH was exercised, and c) Argggh, good
catch, it's perfectly value for the sdch manager to be null, and now that I look
closely, at least one of the try job failures is tripping over that nullity when
copying the URLRequestContext.  Ok, that means I probably won't get failures in
the try jobs and probably will want to land this, once I fix that braino.  New
PS coming soon.

[mmenke, 2015-03-11 17:55:45]

On 2015/03/11 17:47:38, rdsmith wrote:
> On 2015/03/11 17:43:10, mmenke wrote:
> > On 2015/03/11 17:28:33, rdsmith wrote:
> > > Matt: I *am* getting what look to be real try job failures in this test,
> which
> > > I'm excited about, but I'd like you to do a quick review anyway.  No need
to
> > > scan for nits or similar--I don't currently plan to land this--but if you
> > could
> > > just verify that you think CHECK failures are indeed likely to be
> bugs/things
> > > wrong with the code, I would be very grateful.  I'm fairly sure, but I'd
> like
> > to
> > > be very sure.
> > 
> > Hrm...The CL isn't quite running the check I had in mind (Which is fine),
but
> > I'm having trouble figuring out how it's causing a crash in tests that don't
> > exercise SDCH.  In url_request_http_job.cc, if the SDCH manager is deleted,
it
> > should be NULL, so we never call into it.
> 
> So a) What check did you have in mind?  I tried to implement the spirit of
> everything I remember you saying, so if there's something missing, I'd like to
> look into it.

Something like:

URLRequestContext::set_sdch_manager(weak_manager_ptr) {
  weak_manager_ptr_ = weak_manager_ptr;
  if (weak_manager_ptr_.get())
    had_manager_ = true;
}

URLRequestContext::CopyFrom(context) {
  ...
  set_sdch_manager(context->weak_manager_ptr_);
  // Context could be created after the original manager was torn down, though
unlikely.
  had_manager_ = context->had_manager_;
  ...
}

URLRequestHttpJob::OnHeadersReceiverOrWhatever() {
  ..
  if (!context->manager())
    CHECK(!context->had_manager());
  if (context->manager()) {
    // Do normal SDCH stuff.
  }
  ..
}

If all of the old crashes switch from the "Do normal SDCH stuff" block to the
new CHECK, we have confirmation that the issue is URLRequestContexts being used
after the SDCH manager has been destroyed.  If not, the issue is presumably
something else.  We could also distinguish between the main context and ones
created from copy from, if we wanted, to get an extra bit of information.


> b) I had thought the narrowing of checks from the previous CL was
> to focus just on those areas where SDCH was exercised, and c) Argggh, good
> catch, it's perfectly value for the sdch manager to be null, and now that I
look
> closely, at least one of the try job failures is tripping over that nullity
when
> copying the URLRequestContext.  Ok, that means I probably won't get failures
in
> the try jobs and probably will want to land this, once I fix that braino.  New
> PS coming soon.

[mmenke, 2015-03-11 17:59:28]

On 2015/03/11 17:55:45, mmenke wrote:
> On 2015/03/11 17:47:38, rdsmith wrote:
> > On 2015/03/11 17:43:10, mmenke wrote:
> > > On 2015/03/11 17:28:33, rdsmith wrote:
> > > > Matt: I *am* getting what look to be real try job failures in this test,
> > which
> > > > I'm excited about, but I'd like you to do a quick review anyway.  No
need
> to
> > > > scan for nits or similar--I don't currently plan to land this--but if
you
> > > could
> > > > just verify that you think CHECK failures are indeed likely to be
> > bugs/things
> > > > wrong with the code, I would be very grateful.  I'm fairly sure, but I'd
> > like
> > > to
> > > > be very sure.
> > > 
> > > Hrm...The CL isn't quite running the check I had in mind (Which is fine),
> but
> > > I'm having trouble figuring out how it's causing a crash in tests that
don't
> > > exercise SDCH.  In url_request_http_job.cc, if the SDCH manager is
deleted,
> it
> > > should be NULL, so we never call into it.
> > 
> > So a) What check did you have in mind?  I tried to implement the spirit of
> > everything I remember you saying, so if there's something missing, I'd like
to
> > look into it.
> 
> Something like:
> 
> URLRequestContext::set_sdch_manager(weak_manager_ptr) {
>   weak_manager_ptr_ = weak_manager_ptr;
>   if (weak_manager_ptr_.get())
>     had_manager_ = true;
> }
> 
> URLRequestContext::CopyFrom(context) {
>   ...
>   set_sdch_manager(context->weak_manager_ptr_);
>   // Context could be created after the original manager was torn down, though
> unlikely.
>   had_manager_ = context->had_manager_;
>   ...
> }
> 
> URLRequestHttpJob::OnHeadersReceiverOrWhatever() {
>   ..
>   if (!context->manager())
>     CHECK(!context->had_manager());
>   if (context->manager()) {
>     // Do normal SDCH stuff.
>   }
>   ..
> }
> 
> If all of the old crashes switch from the "Do normal SDCH stuff" block to the
> new CHECK, we have confirmation that the issue is URLRequestContexts being
used
> after the SDCH manager has been destroyed.  If not, the issue is presumably
> something else.  We could also distinguish between the main context and ones
> created from copy from, if we wanted, to get an extra bit of information.

Actually, that's not needed...  InvalidateContextGettersOnIO ensures that we get
another CHECK failure after SdchManager has been shut down (Was looking at some
crashes there earlier this week...coincidentally).

[Randy Smith (Not in Mondays), 2015-03-11 20:05:20]

Yeah, that's pretty close to what I came up with after fixing my braino around
the validity of a null sdch_manager_.  Rather than put a check in
url_request_http_job.cc, I just made any call to sdch_manager() CHECK() to make
sure the value of have_sdch_manager_ and sdch_manager_ were in alignment.  I
expect this to pass try jobs (though we'll see :-}); PTAL?

[mmenke, 2015-03-11 20:16:37]

This LGTM, just one comment.

https://codereview.chromium.org/998803003/diff/60001/net/url_request/url_requ...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/998803003/diff/60001/net/url_request/url_requ...
net/url_request/url_request_context.cc:66: CHECK(other->sdch_manager_.get());
If you're going to have a CHECK here, you can just use:
set_sdch_manager(sdch_manager_.get());

And not copy either variable directly.  The CHECK catches the case where
set_sdch_manager wouldn't be sufficient.

[Randy Smith (Not in Mondays), 2015-03-11 20:30:45]

I also added in a reset of the weak pointer if set_sdch_manager() was passed a
null manager.  I don't think that requires a re-review; I'll chase with a
followup CL if I missed something.  Thanks!

https://codereview.chromium.org/998803003/diff/60001/net/url_request/url_requ...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/998803003/diff/60001/net/url_request/url_requ...
net/url_request/url_request_context.cc:66: CHECK(other->sdch_manager_.get());
On 2015/03/11 20:16:36, mmenke wrote:
> If you're going to have a CHECK here, you can just use:
> set_sdch_manager(sdch_manager_.get());
> 
> And not copy either variable directly.  The CHECK catches the case where
> set_sdch_manager wouldn't be sufficient.

Good point; done.

[Randy Smith (Not in Mondays), 2015-03-11 20:31:03]

The CQ bit was checked by rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2015-03-11 20:31:03]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/998803003/#ps80001 (title:
"Incorporated comments and fixed else clause.")

[commit-bot: I haz the power, 2015-03-11 20:31:28]

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

[commit-bot: I haz the power, 2015-03-11 22:15:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-11 22:16:32]

Patchset 5 (id:??) landed as
https://crrev.com/4733facd42dbafe77042ec65e8d6f63fb0e2977a
Cr-Commit-Position: refs/heads/master@{#320164}