Protect WebURLLoaderImpl::Context while receiving responses.

Protect WebURLLoaderImpl::Context while receiving responses.

A client's didReceiveResponse can cancel a request; by protecting the
Context we avoid a use after free in this case.

Interestingly, we really had very good warning about this problem, see 
https://codereview.chromium.org/11900002/ back in January.

R=darin
BUG=241139
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202821

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

[gavinp, 2013-05-22 15:19:29]

darin: Can you PTAL at this? I chose you as an OWNER in both content/ and
webkit/.

sky, japhat: FYI.

[gavinp, 2013-05-23 14:37:06]

+inferno: FYI.

[darin (slow to review), 2013-05-23 14:42:09]

https://codereview.chromium.org/15738007/diff/1/webkit/glue/weburlloader_impl.cc
File webkit/glue/weburlloader_impl.cc (right):

https://codereview.chromium.org/15738007/diff/1/webkit/glue/weburlloader_impl...
webkit/glue/weburlloader_impl.cc:654: scoped_refptr<Context> protect(this);
This looks like it'll do the trick to me.  However, I'm not a big fan of
protect(this) patterns.  I usually try to find some way to avoid it if I can. 
Maybe the caller of OnReceivedResponse can retain a reference, etc.  (I need to
study the code more...)

[inferno, 2013-05-23 15:48:27]

On 2013/05/23 14:42:09, darin wrote:
>
https://codereview.chromium.org/15738007/diff/1/webkit/glue/weburlloader_impl.cc
> File webkit/glue/weburlloader_impl.cc (right):
> 
>
https://codereview.chromium.org/15738007/diff/1/webkit/glue/weburlloader_impl...
> webkit/glue/weburlloader_impl.cc:654: scoped_refptr<Context> protect(this);
> This looks like it'll do the trick to me.  However, I'm not a big fan of
> protect(this) patterns.  I usually try to find some way to avoid it if I can. 
> Maybe the caller of OnReceivedResponse can retain a reference, etc.  (I need
to
> study the code more...)

Please feel free to look at the complete stacktrace -
https://cluster-fuzz.appspot.com/testcase?key=184660605. If there is more than
one caller, we need to be careful about making sure all of them are covered. As
i am looking at stacktrace, another solution might be to have PendingRequestInfo
keep a strong ref here -
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/res...

[gavinp, 2013-05-28 16:25:35]

https://codereview.chromium.org/15738007/diff/1/webkit/glue/weburlloader_impl.cc
File webkit/glue/weburlloader_impl.cc (right):

https://codereview.chromium.org/15738007/diff/1/webkit/glue/weburlloader_impl...
webkit/glue/weburlloader_impl.cc:654: scoped_refptr<Context> protect(this);
On 2013/05/23 14:42:09, darin wrote:
> This looks like it'll do the trick to me.  However, I'm not a big fan of
> protect(this) patterns.  I usually try to find some way to avoid it if I can. 
> Maybe the caller of OnReceivedResponse can retain a reference, etc.  (I need
to
> study the code more...)

Certainly the caller could, but that would be a bigger change to the code. The
typical call path is ResourceDispatcher::OnReceivedResponse calls through the
ResourceLoaderBridge::Peer interface, which Context implements.

Context is refcounted, but ResourceLoaderBridge::Peer is not.

I've spent some time playing with it, and I'm more comfortable guarding here
than making the abstract class Refcounted. I'll make an upload that does that
for comparison if you like, in the meantime I'd welcome your opinion.

[gavinp, 2013-05-28 20:16:30]

Darin,

I've read WebURLLoader a bit more carefully; it's a lot of manual lifetime
management with AddRef() and Release(). In fact, this patch, if landed, will be
the first scoped_refptr<> of the object.

Given that the high level interface, ResourceLoaderBridge::Peer isn't
refcounted, and adding it there would be a big change, and given that the
lifetime management is entirely internal anyway, I'm inclined to agree with your
general reasoning about self references, but say this one is an exception.

What do you say?

[darin (slow to review), 2013-05-28 22:51:07]

OK, LGTM

I think this system is due for an overhaul anyways.  Once we switch to
content_shell for running layout tests and can delete DRT, I hope to be able to
eliminate some layers of indirection here.

[commit-bot: I haz the power, 2013-05-28 22:57:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/15738007/1

[commit-bot: I haz the power, 2013-05-29 08:26:08]

Change committed as 202821

[gavinp, 2013-05-29 18:39:36]

On 2013/05/29 08:26:08, I haz the power (commit-bot) wrote:
> Change committed as 202821

Re opening, as it was reverted in r202835

WebKitBrowserTest.ErrorBodyNoCrash: 
[953:976:0529/031827:4584010225:WARNING:proxy_service.cc(888)] PAC support
disabled because there is no system implementation
HTTP server started on 127.0.0.1:36516...
sending server_data: {"host": "127.0.0.1", "port": 36516} (36 bytes)
ASSERTION FAILED: m_state == Initialized
../../third_party/WebKit/Source/core/loader/ResourceLoader.cpp(358) : virtual
void WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, const
char*, int, int)
1   0xb21e8690
2   0xb1a6f7c5
3   0xb62fb52f
4   0xaec96299 content::ResourceDispatcher::OnReceivedData(IPC::Message const&,
int, int, int, int)
5   0xaec98d89 bool
ResourceMsg_DataReceived::Dispatch<content::ResourceDispatcher,
content::ResourceDispatcher, int, int, int, int>(IPC::Message const*,
content::ResourceDispatcher*, content::ResourceDispatcher*, void
(content::ResourceDispatcher::*)(IPC::Message const&, int, int, int, int))
6   0xaec97476 content::ResourceDispatcher::DispatchMessage(IPC::Message const&)
7   0xaec95b13 content::ResourceDispatcher::OnMessageReceived(IPC::Message
const&)
8   0xaeb2be71 content::ChildThread::OnMessageReceived(IPC::Message const&)
9   0xb4714293 IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message
const&)
10  0xb4717b4e
11  0xb47176e1
12  0xb4717085
13  0xb5d8c644
14  0xb5dd7a5c base::MessageLoop::RunTask(base::PendingTask const&)
15  0xb5dd7b4a base::MessageLoop::DeferOrRunPendingTask(base::PendingTask
const&)
16  0xb5dd83e1 base::MessageLoop::DoWork()
17  0xb5ddfdfe
18  0xb5dd7692 base::MessageLoop::RunInternal()
19  0xb5dd756b base::MessageLoop::RunHandler()
20  0xb5e0e1b8 base::RunLoop::Run()
21  0xb5dd6e98 base::MessageLoop::Run()
22  0xb472710f
IPC::SyncChannel::WaitForReplyWithNestedMessageLoop(IPC::SyncChannel::SyncContext*)
23  0xb4726f82 IPC::SyncChannel::WaitForReply(IPC::SyncChannel::SyncContext*,
base::WaitableEvent*)
24  0xb4726ea1 IPC::SyncChannel::SendWithTimeout(IPC::Message*, int)
25  0xb4726a81 IPC::SyncChannel::Send(IPC::Message*)
26  0xaeb2b97e content::ChildThread::Send(IPC::Message*)
27  0xaed94366 content::RenderThreadImpl::Send(IPC::Message*)
28  0xaedd42b5 content::RenderWidget::Send(IPC::Message*)
29  0xaedb2efe content::RenderViewImpl::Send(IPC::Message*)
30  0xaeda74b1
content::RenderViewImpl::SendAndRunNestedMessageLoop(IPC::SyncMessage*)
31  0xaedaa545 content::RenderViewImpl::runModal()
../../content/browser/webkit_browsertest.cc:91: Failure
Value of: shell()->web_contents()->IsCrashed()
Actual: true
Expected: false

[gavinp, 2013-05-30 15:40:21]

On 2013/05/29 18:39:36, gavinp wrote:
> On 2013/05/29 08:26:08, I haz the power (commit-bot) wrote:
> > Change committed as 202821
> 
> Re opening, as it was reverted in r202835
> 
> WebKitBrowserTest.ErrorBodyNoCrash: 
> [953:976:0529/031827:4584010225:WARNING:proxy_service.cc(888)] PAC support
> disabled because there is no system implementation
> HTTP server started on 127.0.0.1:36516...
> sending server_data: {"host": "127.0.0.1", "port": 36516} (36 bytes)
> ASSERTION FAILED: m_state == Initialized
> ../../third_party/WebKit/Source/core/loader/ResourceLoader.cpp(358) : virtual
> void WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, const
> char*, int, int)
> 1   0xb21e8690
> 2   0xb1a6f7c5
> 3   0xb62fb52f
> 4   0xaec96299 content::ResourceDispatcher::OnReceivedData(IPC::Message
const&,
> int, int, int, int)
> 5   0xaec98d89 bool
> ResourceMsg_DataReceived::Dispatch<content::ResourceDispatcher,
> content::ResourceDispatcher, int, int, int, int>(IPC::Message const*,
> content::ResourceDispatcher*, content::ResourceDispatcher*, void
> (content::ResourceDispatcher::*)(IPC::Message const&, int, int, int, int))
> 6   0xaec97476 content::ResourceDispatcher::DispatchMessage(IPC::Message
const&)
> 7   0xaec95b13 content::ResourceDispatcher::OnMessageReceived(IPC::Message
> const&)
> 8   0xaeb2be71 content::ChildThread::OnMessageReceived(IPC::Message const&)
> 9   0xb4714293 IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message
> const&)
> 10  0xb4717b4e
> 11  0xb47176e1
> 12  0xb4717085
> 13  0xb5d8c644
> 14  0xb5dd7a5c base::MessageLoop::RunTask(base::PendingTask const&)
> 15  0xb5dd7b4a base::MessageLoop::DeferOrRunPendingTask(base::PendingTask
> const&)
> 16  0xb5dd83e1 base::MessageLoop::DoWork()
> 17  0xb5ddfdfe
> 18  0xb5dd7692 base::MessageLoop::RunInternal()
> 19  0xb5dd756b base::MessageLoop::RunHandler()
> 20  0xb5e0e1b8 base::RunLoop::Run()
> 21  0xb5dd6e98 base::MessageLoop::Run()
> 22  0xb472710f
>
IPC::SyncChannel::WaitForReplyWithNestedMessageLoop(IPC::SyncChannel::SyncContext*)
> 23  0xb4726f82 IPC::SyncChannel::WaitForReply(IPC::SyncChannel::SyncContext*,
> base::WaitableEvent*)
> 24  0xb4726ea1 IPC::SyncChannel::SendWithTimeout(IPC::Message*, int)
> 25  0xb4726a81 IPC::SyncChannel::Send(IPC::Message*)
> 26  0xaeb2b97e content::ChildThread::Send(IPC::Message*)
> 27  0xaed94366 content::RenderThreadImpl::Send(IPC::Message*)
> 28  0xaedd42b5 content::RenderWidget::Send(IPC::Message*)
> 29  0xaedb2efe content::RenderViewImpl::Send(IPC::Message*)
> 30  0xaeda74b1
> content::RenderViewImpl::SendAndRunNestedMessageLoop(IPC::SyncMessage*)
> 31  0xaedaa545 content::RenderViewImpl::runModal()
> ../../content/browser/webkit_browsertest.cc:91: Failure
> Value of: shell()->web_contents()->IsCrashed()
> Actual: true
> Expected: false

Blink has regressed. See https://codereview.chromium.org/15725010/ , which can
land after this.

[gavinp, 2013-06-03 13:39:44]

https://chromiumcodereview.appspot.com/15725010/ has now landed in blink, we're
waiting for blink to garden up to 151609 or better.

That happened an hour ago, so we can go CQ...

[commit-bot: I haz the power, 2013-06-03 13:39:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/15738007/1

[commit-bot: I haz the power, 2013-06-03 21:53:04]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-06-03 22:02:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/15738007/1

[commit-bot: I haz the power, 2013-06-04 12:35:57]

Change committed as 203935