Fix cancellation of a pair of URLRequestJob subclasses

Fix cancellation of a pair of URLRequestJob subclasses

URLRequestSimpleJob and URLRequestChromeJob can call back into their
parent URLRequestJob class after Kill() has been invoked, which can
cause Bad Things.

Longer term, we should think about the interface between URLRequestJob
and its subclasses, to see if we can create a more robust interface.

BUG=508900
Committed: https://crrev.com/363ce98af274c07f43acac74b7ca7c23641c996e
Cr-Commit-Position: refs/heads/master@{#342211}

[mmenke, 2015-07-31 21:48:04]

Patchset #4 (id:60001) has been deleted

[mmenke, 2015-08-03 16:39:24]

Patchset #4 (id:80001) has been deleted

[mmenke, 2015-08-03 17:37:03]

Patchset #4 (id:100001) has been deleted

[mmenke, 2015-08-03 17:37:13]

Patchset #4 (id:100001) has been deleted

[mmenke, 2015-08-03 18:48:30]

Patchset #5 (id:140001) has been deleted

[mmenke, 2015-08-03 21:00:25]

Patchset #4 (id:120001) has been deleted

[mmenke, 2015-08-03 21:01:20]

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

[mmenke, 2015-08-03 21:01:20]

URLRequestJob's API with respect to its subclasses makes me sad.  :(

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_http_job_unittest.cc (left):

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:395: // newly-created
transactions after the first one.
It doesn't look to me like this case ever actually happens - jobs don't create
two transactions.  Killing a job and then starting it again certainly never
happens, or if it does, it's a bug.

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
net/url_request/url_request_job.cc:379: return;
These three checks seem aimed at papering over buggy subclasses that don't
respect Kill().  That goes for pretty much all of the !request_ checks in this
file, actually.

Thinking we may want to turn them into CHECKs, see what triggers them, and see
if we can fix the code, rather than have these all over the place.  Or we could
make those checks more hacky, but more robust - either detach the request on
cancellation (Which seems likely to break consumers), or add in cancellation
checks whever we check request is not NULL, which also seems pretty bad.

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
net/url_request/url_request_job.cc:769: request_->status().is_io_pending());
Want to change the added DCHECKs to CHECKs (Particularly this one) in a followup
CL, and see what we turn up.  Would, of course, revert that change at some point
before it hits stable.

[mmenke, 2015-08-03 21:02:52]

Sending this to you mostly because these bugs potentially surface as a CHECK in
AsyncResourceHandler, and there's no obvious reviewer for this stuff.

[davidben, 2015-08-05 18:56:39]

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:138:
base::WeakPtr<URLRequestChromeJob> AsWeakPtr();
Yuck. :-(

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend_unittest.cc (right):

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
content/browser/webui/url_data_manager_backend_unittest.cc:30: }
Do you want to override OnReadCompleted like the other one, to assert we read -1
bytes?

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
content/browser/webui/url_data_manager_backend_unittest.cc:82: // Check that the
URLRequestJob isn't passed headers after cancellation.
URLRequestJob -> URLRequest?

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
content/browser/webui/url_data_manager_backend_unittest.cc:93: // Check that the
URLRequestJob isn't passed data after cancellation.
Ditto.

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
net/url_request/url_request_job.cc:379: return;
Clearly we should unrefcount URLRequestJob. :-) That won't take forever.
(Actually, perhaps we should queue that for the next fixit?)

(I don't have enough of this class swapped in to be able to usefully comment
CHECKs and cancels and stuff.)

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
net/url_request/url_request_job.cc:769: request_->status().is_io_pending());
On 2015/08/03 21:01:20, mmenke wrote:
> Want to change the added DCHECKs to CHECKs (Particularly this one) in a
followup
> CL, and see what we turn up.  Would, of course, revert that change at some
point
> before it hits stable.

SGTM.

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
net/url_request/url_request_simple_job_unittest.cc:68: class
CancelURLRequestDelegate : public TestDelegate {
Any reason this CancelURLRequestDelegate calls Read itself while the
url_data_manager_backend_unittest leans on TestDelegate?

[davidben, 2015-08-05 18:57:08]

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:138:
base::WeakPtr<URLRequestChromeJob> AsWeakPtr();
On 2015/08/05 18:56:39, David Benjamin wrote:
> Yuck. :-(

[To be clear, nothing actionable. I found the place where we call it and fixing
that probably needn't be done in this CL.]

[mmenke, 2015-08-05 19:56:59]

Patchset #5 (id:180001) has been deleted

[mmenke, 2015-08-05 19:57:11]

Patchset #6 (id:220001) has been deleted

[mmenke, 2015-08-05 20:00:25]

THanks for the review!

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
content/browser/webui/url_data_manager_backend.cc:138:
base::WeakPtr<URLRequestChromeJob> AsWeakPtr();
On 2015/08/05 18:57:08, David Benjamin wrote:
> On 2015/08/05 18:56:39, David Benjamin wrote:
> > Yuck. :-(
> 
> [To be clear, nothing actionable. I found the place where we call it and
fixing
> that probably needn't be done in this CL.]

I completely agree on both counts.  The way the 3 classes are intertwined here
is really ugly.  Spent a little time looking for a really simple fix for this,
but seemed like it would require more work than I'm prepared to spend on it.

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
File content/browser/webui/url_data_manager_backend_unittest.cc (right):

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
content/browser/webui/url_data_manager_backend_unittest.cc:30: }
On 2015/08/05 18:56:39, David Benjamin wrote:
> Do you want to override OnReadCompleted like the other one, to assert we read
-1
> bytes?

Done.  I've gone ahead and make the classes identical (Except for the runloop,
which the other class doesn't really need, but given the extra thread hop that
test uses, to make sure no action is pending, I'm happier keeping it there)

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
content/browser/webui/url_data_manager_backend_unittest.cc:82: // Check that the
URLRequestJob isn't passed headers after cancellation.
On 2015/08/05 18:56:39, David Benjamin wrote:
> URLRequestJob -> URLRequest?

Done.

https://codereview.chromium.org/1271593002/diff/160001/content/browser/webui/...
content/browser/webui/url_data_manager_backend_unittest.cc:93: // Check that the
URLRequestJob isn't passed data after cancellation.
On 2015/08/05 18:56:39, David Benjamin wrote:
> Ditto.

Done.

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
net/url_request/url_request_job.cc:379: return;
On 2015/08/05 18:56:39, David Benjamin wrote:
> Clearly we should unrefcount URLRequestJob. :-) That won't take forever.
> (Actually, perhaps we should queue that for the next fixit?)
> 
> (I don't have enough of this class swapped in to be able to usefully comment
> CHECKs and cancels and stuff.)

I agree with removing refcounting, but that wouldn't help with a lot of the
issues here - we keep the URLRequestJob on cancel.  We'd have to change that,
too, to fix a lot of these.

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/1271593002/diff/160001/net/url_request/url_re...
net/url_request/url_request_simple_job_unittest.cc:68: class
CancelURLRequestDelegate : public TestDelegate {
On 2015/08/05 18:56:39, David Benjamin wrote:
> Any reason this CancelURLRequestDelegate calls Read itself while the
> url_data_manager_backend_unittest leans on TestDelegate?

Carelessness on my part, while iterating to get the two tests to fail a bit
better on regression.  I've made them identical, except the RunLoop (It's not
strictly needed in the other test, but given the thread hopping there, I'm
happier having it.

[davidben, 2015-08-06 19:09:48]

lgtm

[mmenke, 2015-08-06 19:26:02]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2015-08-06 19:27:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1271593002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1271593002/240001

[commit-bot: I haz the power, 2015-08-06 21:08:38]

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

[commit-bot: I haz the power, 2015-08-06 21:08:40]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[mmenke, 2015-08-06 21:10:48]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2015-08-06 21:11:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1271593002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1271593002/240001

[commit-bot: I haz the power, 2015-08-06 22:07:44]

Committed patchset #6 (id:240001)

[commit-bot: I haz the power, 2015-08-06 22:08:34]

Patchset 6 (id:??) landed as
https://crrev.com/363ce98af274c07f43acac74b7ca7c23641c996e
Cr-Commit-Position: refs/heads/master@{#342211}

[Peter Kasting, 2015-08-07 01:22:10]

A revert of this CL (patchset #6 id:240001) has been created in
https://codereview.chromium.org/1273813005/ by pkasting@chromium.org.

The reason for reverting is: Caused crashes in the following tests due to DCHECK
failures:

http/tests/serviceworker/registration.html
http/tests/serviceworker/chromium/register-error-messages.html

See e.g.
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20(dbg)...
for a log..