Fix a crash in URLFetcherCore by handling a case where |request_| is NULL

Fix a crash in URLFetcherCore by handling a case where |request_| is NULL

This is a speculative fix for the crash:
https://crash-old.corp.google.com/reportdetail?reportid=4897b184b09b23c8#crashing_thread

My hypothesis is that the request is released while there is a pending call to DidWriteBuffer().
The request was maybe released because
- the fetch was stopped or cancelled
- or ReleaseRequest() was called in OnReadCompleted


BUG=308626, 314241
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232929

[droger, 2013-10-31 12:15:21]

ping

we'd like to have this crash fixed. I am not sure if I correctly identified the
bug nor if this fix is appropriate. Feedback welcome.

[akalin, 2013-10-31 16:13:34]

Sorry, missed this yesterday.

I'm not sure if this is the right fix. Matt, you might know this code better
than I do. Is it legit that request_ is NULL in this case?

[mmenke, 2013-10-31 16:22:37]

On 2013/10/31 16:13:34, akalin wrote:
> Sorry, missed this yesterday.
> 
> I'm not sure if this is the right fix. Matt, you might know this code better
> than I do. Is it legit that request_ is NULL in this case?

Droger:  I think you're probably right about the cause, but I want to
investigate it further before signing off on a fix.

[mmenke, 2013-10-31 18:14:18]

https://codereview.chromium.org/46573008/diff/1/net/url_request/url_fetcher_c...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/46573008/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.cc:836: return;
I'd really like to just delete the writer on cancellation - the writer does
nominally support deletion while a request is pending, but I believe there's
another bug in that case - It would work if it had a SequencedTaskRunner.  We
also currently never reset the writer, which makes me a little nervous about
that option.

So...  I think it's OK to check the request instead, but if we go that route,
I'd rather it be in URLFetcherCore::DidWriteBuffer.

[akalin, 2013-10-31 19:10:23]

I'll let Matt handle this review.

[rohitrao (ping after 24h), 2013-11-01 12:03:18]

One question while I have you guys:

How recent is the code in question?  I flagged this because I saw a number of
net stack crashes in M31 iOS builds, but I don't have a sense of how prevalent
this crash will be in stable.  If this code has been around forever, then that's
an indication that I could ship without a fix.  If this code is new, then the
fix is more urgent.

Another question is how risky you think the fix is.  Would you be willing to
pull it last-minute into a stable build?

Thanks!
  Rohit

[mmenke, 2013-11-01 15:31:02]

On 2013/11/01 12:03:18, rohitrao wrote:
> One question while I have you guys:
> 
> How recent is the code in question?  I flagged this because I saw a number of
> net stack crashes in M31 iOS builds, but I don't have a sense of how prevalent
> this crash will be in stable.  If this code has been around forever, then
that's
> an indication that I could ship without a fix.  If this code is new, then the
> fix is more urgent.

The code does have a CL or two a month, it's possible one of them added this
problem, though I don't see anything obvious in the M30-M31 timeframe.  We're
run into threading issues with this class before.  The issue could also be
triggered by a new UrlFetcher consumer than more aggressively cancels requests.

> Another question is how risky you think the fix is.  Would you be willing to
> pull it last-minute into a stable build?

I don't think the fix is terribly risky, but I'm reluctant to land anything on
stable unless three things are true:
1)  We have a perfect understanding of the problem.  I can see how this can
happen, but I don't have a perfect understanding of cancellation behavior on all
code paths.
2)  There's a significant need to land on stable.
3)  We have unit tests reproducing the problem (This feeds into one - can't make
good tests unless you fully understand the issue).

I'm not sure any of these are true, in this case.
I'm not convinced that any of those are true.

[droger, 2013-11-04 15:38:03]

https://codereview.chromium.org/46573008/diff/1/net/url_request/url_fetcher_c...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/46573008/diff/1/net/url_request/url_fetcher_c...
net/url_request/url_fetcher_core.cc:836: return;
On 2013/10/31 18:14:18, mmenke wrote:
> I'd really like to just delete the writer on cancellation - the writer does
> nominally support deletion while a request is pending, but I believe there's
> another bug in that case - It would work if it had a SequencedTaskRunner.  We
> also currently never reset the writer, which makes me a little nervous about
> that option.
> 
> So...  I think it's OK to check the request instead, but if we go that route,
> I'd rather it be in URLFetcherCore::DidWriteBuffer.

Done.

[mmenke, 2013-11-04 15:39:54]

On 2013/11/04 15:38:03, droger wrote:
>
https://codereview.chromium.org/46573008/diff/1/net/url_request/url_fetcher_c...
> File net/url_request/url_fetcher_core.cc (right):
> 
>
https://codereview.chromium.org/46573008/diff/1/net/url_request/url_fetcher_c...
> net/url_request/url_fetcher_core.cc:836: return;
> On 2013/10/31 18:14:18, mmenke wrote:
> > I'd really like to just delete the writer on cancellation - the writer does
> > nominally support deletion while a request is pending, but I believe there's
> > another bug in that case - It would work if it had a SequencedTaskRunner. 
We
> > also currently never reset the writer, which makes me a little nervous about
> > that option.
> > 
> > So...  I think it's OK to check the request instead, but if we go that
route,
> > I'd rather it be in URLFetcherCore::DidWriteBuffer.
> 
> Done.

LGTM

[mmenke, 2013-11-04 15:42:20]

On 2013/11/04 15:39:54, mmenke wrote:
> On 2013/11/04 15:38:03, droger wrote:
> >
>
https://codereview.chromium.org/46573008/diff/1/net/url_request/url_fetcher_c...
> > File net/url_request/url_fetcher_core.cc (right):
> > 
> >
>
https://codereview.chromium.org/46573008/diff/1/net/url_request/url_fetcher_c...
> > net/url_request/url_fetcher_core.cc:836: return;
> > On 2013/10/31 18:14:18, mmenke wrote:
> > > I'd really like to just delete the writer on cancellation - the writer
does
> > > nominally support deletion while a request is pending, but I believe
there's
> > > another bug in that case - It would work if it had a SequencedTaskRunner. 
> We
> > > also currently never reset the writer, which makes me a little nervous
about
> > > that option.
> > > 
> > > So...  I think it's OK to check the request instead, but if we go that
> route,
> > > I'd rather it be in URLFetcherCore::DidWriteBuffer.
> > 
> > Done.
> 
> LGTM

Actually...could you add a short comment about how this happens (Something along
the lines of if cancelled, may have been deleted).

[mmenke, 2013-11-04 15:42:21]

On 2013/11/04 15:39:54, mmenke wrote:
> On 2013/11/04 15:38:03, droger wrote:
> >
>
https://codereview.chromium.org/46573008/diff/1/net/url_request/url_fetcher_c...
> > File net/url_request/url_fetcher_core.cc (right):
> > 
> >
>
https://codereview.chromium.org/46573008/diff/1/net/url_request/url_fetcher_c...
> > net/url_request/url_fetcher_core.cc:836: return;
> > On 2013/10/31 18:14:18, mmenke wrote:
> > > I'd really like to just delete the writer on cancellation - the writer
does
> > > nominally support deletion while a request is pending, but I believe
there's
> > > another bug in that case - It would work if it had a SequencedTaskRunner. 
> We
> > > also currently never reset the writer, which makes me a little nervous
about
> > > that option.
> > > 
> > > So...  I think it's OK to check the request instead, but if we go that
> route,
> > > I'd rather it be in URLFetcherCore::DidWriteBuffer.
> > 
> > Done.
> 
> LGTM

Actually...could you add a short comment about how this happens (Something along
the lines of if cancelled, may have been deleted).

[commit-bot: I haz the power, 2013-11-04 15:42:37]

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

[droger, 2013-11-04 15:55:56]

I added a comment, ptal.

[mmenke, 2013-11-04 15:56:34]

On 2013/11/04 15:55:56, droger wrote:
> I added a comment, ptal.

LGTM

[commit-bot: I haz the power, 2013-11-04 16:03:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/46573008/350001

[commit-bot: I haz the power, 2013-11-04 16:25:01]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-04 17:40:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/46573008/350001

[commit-bot: I haz the power, 2013-11-04 20:26:29]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-04 21:18:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/46573008/350001

[commit-bot: I haz the power, 2013-11-05 01:13:50]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-05 04:02:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/46573008/350001

[commit-bot: I haz the power, 2013-11-05 06:47:07]

Change committed as 232929