Fix WebDataRequest ownership gap

Fix WebDataRequest ownership gap

BUG=166488, 173074
TBR=ben@chromium.org (for gyp changes)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179527

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180019

[Cait (Slow), 2013-01-16 17:03:04]

Hi David,

This is the fix for the ownership gap mentioned in
https://chromiumcodereview.appspot.com/11783091.

Thanks,
Cait

[dhollowa, 2013-01-16 18:41:29]

LGTM.  Thanks.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_request_manager.cc (right):

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:41: }
nit: extra line is needed here.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:97: if (i ==
pending_requests_.end()) {
nit: no longer need {} for single line body.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:106: NOTREACHED() <<
"Canceling a nonexistent web data service request";
nit: no longer need {} for single line body.

[erikwright (departed), 2013-01-16 19:00:23]

Some suggestions.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_request_manager.cc (right):

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:79: i->second->Cancel();
I think it's a requirement that WebDataRequest::Cancel() NULL-out the pointer to
the manager (and that ~WebDataRequest behave correctly at that time).

Otherwise you have a use-after-free.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:100: i->second->Cancel();
It seems like a bug that 'i' is not erased from pending_requests_ here.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:137: request->GetResult());
Make GetResult transfer ownership of the WDTypedResult to the caller (i.e.,
return a scoped_ptr<WDTypedResult>).

Continue to pass the raw pointer to consumers, but that will lead to the result
being freed at the end of this scope. And move the call to
WDTypedResult::Destroy into ~WDTypedResult.

Then the 'else' block can be removed.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:143: result->Destroy();
As an aside, rather than passing an optional callback into WDTResult<T>(), why
not subclass it where it is necessary and implement the callback behaviour in
the subclass's destructor, especially now that the callback would only ever be
invoked from there.

But it would be even better to eliminate the WDTypedResult and WebDataConsumer
classes, replace the consumer with callbacks, and be done with it.

Obviously, one way or the other for a future CL.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:145: }
I think something at this point must tell the request that it is complete,
should release its pointer to the manager, and should not call
MaybeCancelRequest.

For example request->OnComplete() or something.

And therefore, MaybeCancelRequest can be factored out, and ~WebDataRequest can
just call CancelRequest.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_request_manager.h (right):

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.h:103: // Cancel any pending
request (expects pending_requests_ map to contain |h|).
replace 'expects' with something more explicit. Does it crash otherwise? Is
there some notification before which it is OK to call Cancel but after which it
is forbidden?

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.h:111: void
RequestCompletedOnThread(scoped_ptr<WebDataRequest> request);
This can be private, right?

[Cait (Slow), 2013-01-24 22:45:50]

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_request_manager.cc (right):

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:41: }
On 2013/01/16 18:41:29, dhollowa wrote:
> nit: extra line is needed here.

Done.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:79: i->second->Cancel();
On 2013/01/16 19:00:23, erikwright wrote:
> I think it's a requirement that WebDataRequest::Cancel() NULL-out the pointer
to
> the manager (and that ~WebDataRequest behave correctly at that time).
> 
> Otherwise you have a use-after-free.

Done.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:97: if (i ==
pending_requests_.end()) {
On 2013/01/16 18:41:29, dhollowa wrote:
> nit: no longer need {} for single line body.

Done.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:100: i->second->Cancel();
On 2013/01/16 19:00:23, erikwright wrote:
> It seems like a bug that 'i' is not erased from pending_requests_ here.

Done -- This causes a bit of weirdness in the case that the request is cancelled
in between calls to RequestCompleted and ReqCompletedOnThread. We end up in
ReqCompletedOnThread witha request no longer in the pending_requests_. For now
I'm adding an isCancelled check with an early exit.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:106: NOTREACHED() <<
"Canceling a nonexistent web data service request";
On 2013/01/16 18:41:29, dhollowa wrote:
> nit: no longer need {} for single line body.

Done.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:137: request->GetResult());
On 2013/01/16 19:00:23, erikwright wrote:
> Make GetResult transfer ownership of the WDTypedResult to the caller (i.e.,
> return a scoped_ptr<WDTypedResult>).
> 
> Continue to pass the raw pointer to consumers, but that will lead to the
result
> being freed at the end of this scope. And move the call to
> WDTypedResult::Destroy into ~WDTypedResult.
> 
> Then the 'else' block can be removed.

Done.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:143: result->Destroy();
On 2013/01/16 19:00:23, erikwright wrote:
> As an aside, rather than passing an optional callback into WDTResult<T>(), why
> not subclass it where it is necessary and implement the callback behaviour in
> the subclass's destructor, especially now that the callback would only ever be
> invoked from there.
> 
> But it would be even better to eliminate the WDTypedResult and WebDataConsumer
> classes, replace the consumer with callbacks, and be done with it.
> 
> Obviously, one way or the other for a future CL.

Done.

https://codereview.chromium.org/11862010/diff/11001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:145: }
On 2013/01/16 19:00:23, erikwright wrote:
> I think something at this point must tell the request that it is complete,
> should release its pointer to the manager, and should not call
> MaybeCancelRequest.
> 
> For example request->OnComplete() or something.
> 
> And therefore, MaybeCancelRequest can be factored out, and ~WebDataRequest can
> just call CancelRequest.

Done.

[erikwright (departed), 2013-01-28 20:01:05]

LGTM with nits.

More fantastic work. Thank you!

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_results.h (right):

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_results.h:88: const DestroyCallback&
callback)
indentation.

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_request_manager.cc (right):

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:107: NOTREACHED() <<
"Canceling a nonexistent web data service request";
If you are really convinced this can never happen, then just get rid of the 'if'
and do the following:

DCHECK(i != pending_requests_.end()) << "Canceling ... ";

Otherwise, re-insert the early return.

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:125: pending_lock_.Acquire();
Can you go ahead and convert this to an auto-lock? I understand that
'IsCancelled()' can't be invoked while the lock is held for fear of deadlock,
but how about putting lines 126-134 in a {...} block and using an auto-lock in
there.

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:143:
consumer->OnWebDataServiceRequestDone(request->GetHandle(),
This looks like it might not need to wrap.

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_request_manager.h (right):

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.h:59: scoped_ptr<WDTypedResult>
GetResult();
A note that the caller should only invoke this one time.

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.h:62: // Used to notify manager
if request is cancelled.
A note about why a raw pointer is used vs. a ref_ptr

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.h:104: void
RequestCompletedOnThread(scoped_ptr<WebDataRequest> request);
needs to be public?

[Cait (Slow), 2013-01-29 01:10:29]

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_results.h (right):

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_results.h:88: const DestroyCallback&
callback)
On 2013/01/28 20:01:05, erikwright wrote:
> indentation.

Done.

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_request_manager.cc (right):

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:107: NOTREACHED() <<
"Canceling a nonexistent web data service request";
On 2013/01/28 20:01:05, erikwright wrote:
> If you are really convinced this can never happen, then just get rid of the
'if'
> and do the following:
> 
> DCHECK(i != pending_requests_.end()) << "Canceling ... ";
> 
> Otherwise, re-insert the early return.

Done.

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:125: pending_lock_.Acquire();
On 2013/01/28 20:01:05, erikwright wrote:
> Can you go ahead and convert this to an auto-lock? I understand that
> 'IsCancelled()' can't be invoked while the lock is held for fear of deadlock,
> but how about putting lines 126-134 in a {...} block and using an auto-lock in
> there.

Done.

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.cc:143:
consumer->OnWebDataServiceRequestDone(request->GetHandle(),
On 2013/01/28 20:01:05, erikwright wrote:
> This looks like it might not need to wrap.

Done.

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_request_manager.h (right):

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.h:59: scoped_ptr<WDTypedResult>
GetResult();
On 2013/01/28 20:01:05, erikwright wrote:
> A note that the caller should only invoke this one time.

Done.

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.h:62: // Used to notify manager
if request is cancelled.
On 2013/01/28 20:01:05, erikwright wrote:
> A note about why a raw pointer is used vs. a ref_ptr

Done.

https://codereview.chromium.org/11862010/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.h:104: void
RequestCompletedOnThread(scoped_ptr<WebDataRequest> request);
On 2013/01/28 20:01:05, erikwright wrote:
> needs to be public?

Done.

[commit-bot: I haz the power, 2013-01-29 15:27:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11862010/39002

[commit-bot: I haz the power, 2013-01-29 15:28:01]

Presubmit check for 11862010-39002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2013-01-29 15:31:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11862010/39002

[commit-bot: I haz the power, 2013-01-29 16:21:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
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-01-29 17:52:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11862010/39002

[commit-bot: I haz the power, 2013-01-29 18:32:16]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
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-01-29 21:56:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11862010/41005

[commit-bot: I haz the power, 2013-01-29 22:29:12]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-01-29 22:38:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11862010/41005

[commit-bot: I haz the power, 2013-01-30 04:55:54]

Change committed as 179527

[commit-bot: I haz the power, 2013-01-31 21:14:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11862010/34003

[commit-bot: I haz the power, 2013-02-01 00:13:49]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-02-01 00:20:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11862010/34003

[commit-bot: I haz the power, 2013-02-01 00:42:24]

Change committed as 180019