Add debug information for crbug.com/289715

Add debug information for crbug.com/289715

Add temporary debugging information to determine whether a callback happens
after the targeted URLRequestHttpJob was already deleted or whether a callback
happens after its transaction was deleted.

BUG=289715
Committed: https://crrev.com/aec005dce78bc2c2513f4e49343b8baad751a852
Cr-Commit-Position: refs/heads/master@{#310822}

[battre, 2015-01-09 15:56:35]

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

[battre, 2015-01-09 15:57:25]

Hi Matt,

could you please review this (feel free to put into CQ if it LGTY)? I guess this
is the easiest way to figure out what happens.

Thanks,
Dominic

[mmenke, 2015-01-09 16:11:57]

https://codereview.chromium.org/810883007/diff/1/net/url_request/url_request_...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/810883007/diff/1/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:691: CHECK(!destructor_was_called_);
If the destructor was already called, what are the odds of us actually getting
this far?  Also, shouldn't this be in OnHeadersReceivedCallback?

If we want to by more robust, my suggestion is to temporarily make
OnHeadersReceivedCallback static and add a weak ptr factory like this:

static URLRequestHttpJob
OnHeadersReceivedCallback(scoped:ptr<base::WeakPtr<URLRequestHttpJob>> job, int
result) {
  CHECK(**job);
  // Alias transaction_state_ here.
  CHECK((*job)->transaction_);
  // Current OnHeadersReceivedCallback code here, using **job.
}

[battre, 2015-01-09 16:46:14]

I think I have implemented a variation of what you proposed. Please take a look.
Thanks!

https://codereview.chromium.org/810883007/diff/1/net/url_request/url_request_...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/810883007/diff/1/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:691: CHECK(!destructor_was_called_);
On 2015/01/09 16:11:57, mmenke wrote:
> If the destructor was already called, what are the odds of us actually getting
> this far?  Also, shouldn't this be in OnHeadersReceivedCallback?
> 
> If we want to by more robust, my suggestion is to temporarily make
> OnHeadersReceivedCallback static and add a weak ptr factory like this:
> 
> static URLRequestHttpJob
> OnHeadersReceivedCallback(scoped:ptr<base::WeakPtr<URLRequestHttpJob>> job,
int
> result) {
>   CHECK(**job);
>   // Alias transaction_state_ here.
>   CHECK((*job)->transaction_);
>   // Current OnHeadersReceivedCallback code here, using **job.
> }

Done.

[mmenke, 2015-01-09 16:54:51]

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:481: }
nit:  No braces when if and body are both one line.

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:949: void
OnHeadersReceivedCallbackForDebugging(
Definition order should match declaration order (Which is another reason it's
nice to make this a static method).

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:952: CHECK(job.get());
Does this work, or does base::Bind cancel the callback when a weak ptr is NULL? 
Certainly know it does that when the |this| weak ptr is NULL, not sure about
others.

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
net/url_request/url_request_http_job.h:39: int);
optional:  Think this is a little cleaner as a static rather than up here.

[battre, 2015-01-09 18:02:30]

PTAL

Thanks,
Dominic

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:481: }
On 2015/01/09 at 16:54:51, mmenke wrote:
> nit:  No braces when if and body are both one line.

Done.

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:949: void
OnHeadersReceivedCallbackForDebugging(
On 2015/01/09 at 16:54:51, mmenke wrote:
> Definition order should match declaration order (Which is another reason it's
nice to make this a static method).

Done.

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
net/url_request/url_request_http_job.cc:952: CHECK(job.get());
On 2015/01/09 at 16:54:51, mmenke wrote:
> Does this work, or does base::Bind cancel the callback when a weak ptr is
NULL?  Certainly know it does that when the |this| weak ptr is NULL, not sure
about others.

I verified that this:
class AAA {
 public:
  AAA() : weak_factory_(this) {};
  typedef base::Callback<void(int)> CompletionCallback;
  CompletionCallback GetCompletionCallback() {
    return base::Bind(&AAA::Test, weak_factory_.GetWeakPtr());
  }
 private:
  static void Test(base::WeakPtr<AAA> ptr, int value) {
    LOG(ERROR) << ptr.get() << " value " << value;
  }
  base::WeakPtrFactory<AAA> weak_factory_;
};

TEST(AAA, TestAAA) {
  AAA::CompletionCallback callback;
  {
    AAA a;
    callback = a.GetCompletionCallback();
  }
  callback.Run(1);
}

prints

0 value 1

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/810883007/diff/20001/net/url_request/url_requ...
net/url_request/url_request_http_job.h:39: int);
On 2015/01/09 at 16:54:51, mmenke wrote:
> optional:  Think this is a little cleaner as a static rather than up here.

Done.

[mmenke, 2015-01-09 18:04:22]

LGTM!  Thanks for double-checking on the weak ptr thing.

https://codereview.chromium.org/810883007/diff/60001/net/url_request/url_requ...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/810883007/diff/60001/net/url_request/url_requ...
net/url_request/url_request_http_job.h:94: int);
Should name arguments in declaration.

[battre, 2015-01-09 18:05:43]

Thanks.

https://codereview.chromium.org/810883007/diff/60001/net/url_request/url_requ...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/810883007/diff/60001/net/url_request/url_requ...
net/url_request/url_request_http_job.h:94: int);
On 2015/01/09 18:04:21, mmenke wrote:
> Should name arguments in declaration.

Done.

[battre, 2015-01-09 18:06:00]

The CQ bit was checked by battre@chromium.org

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

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

[commit-bot: I haz the power, 2015-01-09 19:38:44]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-09 19:40:18]

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

[battre, 2015-02-09 15:03:48]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/907033002/ by battre@chromium.org.

The reason for reverting is: Remove debugging information that was collected for
crbug.com/289715.