Remove webkit's ResourceLoaderBridge interface.

Remove webkit's ResourceLoaderBridge interface.

ResourceLoaderBridge was originally created to bridge code in //webkit that
talked to WebURLLoaderImpl (which talked to Blink) with code in
ResourceDispatcher in content which used IPC. Now that WebURLLoaderImpl
is in content/child, we don't need this interface anymore.

As requested by John in -
https://codereview.chromium.org/186193005/diff/1/content/public/child/resource_loader_bridge.h#newcode42

BUG=265753, 338338, 237249
TEST=content_unittests
R=jam@chromium.org
TBR=darin

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

[tfarina, 2014-04-05 02:03:45]

https://codereview.chromium.org/226273005/diff/1/content/child/resource_dispa...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/1/content/child/resource_dispa...
content/child/resource_dispatcher.h:98: class IPCResourceLoaderBridge;
John, I burned my head, and this was the way I found to make this work.

I haven't fold IPCResourceLoaderBridge data members and member functions
directly here in ResourceDispatcher because of IPC's ResourceHostMsg_Request
(from content/common/resource_messages.h - which I can't include in
resource_dispatcher.h). Yes, I tried this way initially.

You may help me find a better design for this code and I'm happy to change it
your way.

Thanks in advance for reviewing this.

[jam, 2014-04-07 16:43:22]

https://codereview.chromium.org/226273005/diff/1/content/child/resource_dispa...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/1/content/child/resource_dispa...
content/child/resource_dispatcher.h:98: class IPCResourceLoaderBridge;
On 2014/04/05 02:03:46, tfarina wrote:
> John, I burned my head, and this was the way I found to make this work.
> 
> I haven't fold IPCResourceLoaderBridge data members and member functions
> directly here in ResourceDispatcher because of IPC's ResourceHostMsg_Request
> (from content/common/resource_messages.h - which I can't include in
> resource_dispatcher.h). Yes, I tried this way initially.

you can forward declare that struct and put it into a scoped_ptr
> 
> You may help me find a better design for this code and I'm happy to change it
> your way.
> 
> Thanks in advance for reviewing this.

https://codereview.chromium.org/226273005/diff/20001/content/child/npapi/plug...
File content/child/npapi/plugin_url_fetcher.cc (right):

https://codereview.chromium.org/226273005/diff/20001/content/child/npapi/plug...
content/child/npapi/plugin_url_fetcher.cc:146: dispatcher_ =
ChildThread::current()->resource_dispatcher();
no need to cache ResourceDispatcher into a member variable. get it from
ChildThread every time

https://codereview.chromium.org/226273005/diff/20001/content/child/resource_d...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/20001/content/child/resource_d...
content/child/resource_dispatcher.h:51: void CreateBridge(const RequestInfo&
request_info);
move all the comments from the deleted interface to here

[tfarina, 2014-04-11 01:43:55]

John, sorry for the delay. college tests this week and previous one.

This should be ready now for another review. PTAL.

Thanks,

https://codereview.chromium.org/226273005/diff/1/content/child/resource_dispa...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/1/content/child/resource_dispa...
content/child/resource_dispatcher.h:98: class IPCResourceLoaderBridge;
On 2014/04/07 16:43:22, jam wrote:
> On 2014/04/05 02:03:46, tfarina wrote:
> > John, I burned my head, and this was the way I found to make this work.
> > 
> > I haven't fold IPCResourceLoaderBridge data members and member functions
> > directly here in ResourceDispatcher because of IPC's ResourceHostMsg_Request
> > (from content/common/resource_messages.h - which I can't include in
> > resource_dispatcher.h). Yes, I tried this way initially.
> 
> you can forward declare that struct and put it into a scoped_ptr
> > 
> > You may help me find a better design for this code and I'm happy to change
it
> > your way.
> > 
> > Thanks in advance for reviewing this.
> 

Done.

https://codereview.chromium.org/226273005/diff/20001/content/child/npapi/plug...
File content/child/npapi/plugin_url_fetcher.cc (right):

https://codereview.chromium.org/226273005/diff/20001/content/child/npapi/plug...
content/child/npapi/plugin_url_fetcher.cc:146: dispatcher_ =
ChildThread::current()->resource_dispatcher();
On 2014/04/07 16:43:22, jam wrote:
> no need to cache ResourceDispatcher into a member variable. get it from
> ChildThread every time

Done.

https://codereview.chromium.org/226273005/diff/20001/content/child/resource_d...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/20001/content/child/resource_d...
content/child/resource_dispatcher.h:51: void CreateBridge(const RequestInfo&
request_info);
On 2014/04/07 16:43:22, jam wrote:
> move all the comments from the deleted interface to here

Done.

[tfarina, 2014-04-11 03:00:52]

https://codereview.chromium.org/226273005/diff/330001/content/child/resource_...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/330001/content/child/resource_...
content/child/resource_dispatcher.h:53: void CreateBridge(const RequestInfo&
request_info);
I forgot to ask. John, do you have a better name for this?

[jam, 2014-04-11 18:20:14]

ResourceDispatcher is one-per-process. You're now storing per-request data as a
member variable, which won't work since RD supports more than one request at a
time.

You will need a map in RD from request, and then the methods that you moved from
the bridge interface can take that as a key

[tfarina, 2014-04-11 19:14:38]

John, let me see if I understand your proposal (without you taking this patch
from me, if you don't mind, as I want to finish it myself ;)).

Are you suggesting that I add a map to ResourceDispatcher, and that looks like
this:

std::map<int, ResourceDispatcher> requests_;

?

I'm following the idea from
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/webs...

Will the key be "routing_id"?

If the map value can't be ResourceDispatcher, then is is:

std::map<int, ResourceHostMsg_Request>

?

If it can't also be this way, then will be the map like this:

std::map<int, SomeOtherType>

?

And that SomeOtherType will contain request_, peer_, request_id_, routing_id_,
frame_origin_, is_synchronous_request_?

I hope I'm not being too dumb asking those "trivial" questions. With them I just
want to avoid review cycles and make sure I'm moving towards the right
direction.

[tfarina, 2014-04-15 02:28:56]

John, ping??

[jam, 2014-04-15 15:13:39]

On 2014/04/15 02:28:56, tfarina wrote:
> John, ping??

ResourceDispatcher already has a pending_requests_ map, so we should reuse that.
Let's separate out the sync vs async loading case, since for the former we only
call ResourceDispatcher once and don't need a key to keep calling it again.

sync:
-have a void StartSync method that takes in RequestInfo&, ResourceRequestBody*
and SyncLoadResponse*

async
-have a StartAsync method that takes in RequestInfo&, ResourceRequestBody* and
RequestPeer*. It returns the request_id that's a key to the pending_requests_
map. The Cancel/SetDefersLoading/DidChangePriority methods take that argument as
key.

[tfarina, 2014-04-16 06:58:15]

On 2014/04/15 15:13:39, jam wrote:
> On 2014/04/15 02:28:56, tfarina wrote:
> > John, ping??
> 
> ResourceDispatcher already has a pending_requests_ map, so we should reuse
that.
> Let's separate out the sync vs async loading case, since for the former we
only
> call ResourceDispatcher once and don't need a key to keep calling it again.
> 
> sync:
> -have a void StartSync method that takes in RequestInfo&, ResourceRequestBody*
> and SyncLoadResponse*
> 
Done.

> async
> -have a StartAsync method that takes in RequestInfo&, ResourceRequestBody* and
> RequestPeer*. It returns the request_id that's a key to the pending_requests_
> map. The Cancel/SetDefersLoading/DidChangePriority methods take that argument
as
> key.
Done.

John, I feel I'm missing something. Please, see if I'm on the right direction. I
don't know if I get all the details right, the diff is rather complicated.

[jam, 2014-04-16 16:24:04]

it would be helpful in changes like this to wait for trybots to be green, as
they would probably expose many of the issues that i'm pointing out

https://codereview.chromium.org/226273005/diff/450001/content/child/npapi/plu...
File content/child/npapi/plugin_url_fetcher.cc (right):

https://codereview.chromium.org/226273005/diff/450001/content/child/npapi/plu...
content/child/npapi/plugin_url_fetcher.cc:152: request_id_ =
dispatcher->StartAsync(request_info, request_body, this);
the StartAsync needs to move out of this ifdef, otherwise you're only starting
it if there's data to send

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:89: }
ditto: remove this section

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:513: if (request_id_ != -1) {
ditto: there should be no per-request state in RD

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:551: if (request_id_ != -1) {
ditto

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:559: request_id_ = AddPendingRequest(peer,
ditto

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:571: void ResourceDispatcher::Cancel(int
request_id) {
actually no need to have Cancel method, since CancelPendingRequest does the same
thing.

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.h:233: bool is_synchronous_request_;
remove this entire section (see my second-last message)

[tfarina, 2014-04-16 22:33:12]

RoundTrip is crashing:

[ RUN      ] ResourceDispatcherTest.RoundTrip
Received signal 11 SEGV_MAPERR ffffffff01cffcf8
#0 0x7ff7f718675e base::debug::StackTrace::StackTrace()
#1 0x7ff7f7186297 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7ff7e973ccb0 <unknown>
#3 0x00000054f68c std::string::_Rep::_M_is_leaked()
#4 0x00000054f5d4 std::string::_Rep::_M_grab()
#5 0x00000054f563 std::string::string()
#6 0x7ff7f745b72b GURL::GURL()
#7 0x7ff7edddb8a3 content::RequestExtraData::frame_origin()
#8 0x7ff7eddd9cff content::ResourceDispatcher::CreateRequest()
#9 0x7ff7eddd9d70 content::ResourceDispatcher::StartAsync()
#10 0x000000d7593f content::ResourceDispatcherTest_RoundTrip_Test::TestBody()
#11 0x000001237d13 testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#12 0x0000012266ce testing::internal::HandleExceptionsInMethodIfSupported<>()
#13 0x00000121d395 testing::Test::Run()
#14 0x00000121daab testing::TestInfo::Run()
#15 0x00000121e09a testing::TestCase::Run()
#16 0x0000012226a8 testing::internal::UnitTestImpl::RunAllTests()
#17 0x000001231d23 testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#18 0x0000012281de testing::internal::HandleExceptionsInMethodIfSupported<>()
#19 0x000001222334 testing::UnitTest::Run()
#20 0x0000011cfee1 RUN_ALL_TESTS()
#21 0x0000011cef77 base::TestSuite::Run()
#22 0x00000114e36d content::UnitTestTestSuite::Run()
#23 0x000000f0cbe2 base::internal::RunnableAdapter<>::Run()
#24 0x000000f0cb4c base::internal::InvokeHelper<>::MakeItSo()
#25 0x000000f0cafa base::internal::Invoker<>::Run()
#26 0x0000011c7cbe base::Callback<>::Run()
#27 0x0000011c46de base::(anonymous namespace)::LaunchUnitTestsInternal()
#28 0x0000011c43d0 base::LaunchUnitTests()
#29 0x000000f0c8c0 main
#30 0x7ff7e8e7476d __libc_start_main
#31 0x000000541ff5 <unknown>
  r8: 00007fffd7d16ee0  r9: 0000000000100000 r10: 0000000000000000 r11:
00007ff7e8edf2e5
 r12: 0000000000541fcc r13: 00007fffd7d17f60 r14: 0000000000000000 r15:
0000000000000000
  di: ffffffff01cffce8  si: 00007fffd7d16b38  bp: 00007fffd7d16ac0  bx:
0000000000000000
  dx: ffffffff01cffce8  ax: 00007fffd7d16b30  cx: 0000000000000000  sp:
00007fffd7d16ac0
  ip: 000000000054f68c efl: 0000000000010202 cgf: 0000000000000033 erf:
0000000000000005
 trp: 000000000000000e msk: 0000000000000000 cr2: ffffffff01cffcf8
[0416/192937:ERROR:kill_posix.cc(191)] Unable to terminate process group 14174:
No such process
[1/5] ResourceDispatcherTest.RoundTrip (CRASHED)

Running trybots now.

https://codereview.chromium.org/226273005/diff/450001/content/child/npapi/plu...
File content/child/npapi/plugin_url_fetcher.cc (right):

https://codereview.chromium.org/226273005/diff/450001/content/child/npapi/plu...
content/child/npapi/plugin_url_fetcher.cc:152: request_id_ =
dispatcher->StartAsync(request_info, request_body, this);
On 2014/04/16 16:24:04, jam wrote:
> the StartAsync needs to move out of this ifdef, otherwise you're only starting
> it if there's data to send

Done.

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:89: }
On 2014/04/16 16:24:04, jam wrote:
> ditto: remove this section

Done.

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:89: }
On 2014/04/16 16:24:04, jam wrote:
> ditto: remove this section

Done.

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:513: if (request_id_ != -1) {
On 2014/04/16 16:24:04, jam wrote:
> ditto: there should be no per-request state in RD

Done.

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:513: if (request_id_ != -1) {
On 2014/04/16 16:24:04, jam wrote:
> ditto: there should be no per-request state in RD

Done.

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:551: if (request_id_ != -1) {
On 2014/04/16 16:24:04, jam wrote:
> ditto

Done.

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:559: request_id_ = AddPendingRequest(peer,
On 2014/04/16 16:24:04, jam wrote:
> ditto

Done.

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:559: request_id_ = AddPendingRequest(peer,
On 2014/04/16 16:24:04, jam wrote:
> ditto

Done.

https://codereview.chromium.org/226273005/diff/450001/content/child/resource_...
content/child/resource_dispatcher.cc:571: void ResourceDispatcher::Cancel(int
request_id) {
On 2014/04/16 16:24:04, jam wrote:
> actually no need to have Cancel method, since CancelPendingRequest does the
same
> thing.

Done.

[tfarina, 2014-04-18 00:14:45]

John, are you waiting on me to fix the crash to do another pass?

[tfarina, 2014-04-18 02:31:45]

I will get back to you when I have the following comments sorted out.

https://codereview.chromium.org/226273005/diff/490001/content/child/resource_...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/490001/content/child/resource_...
content/child/resource_dispatcher.h:219: scoped_ptr<ResourceHostMsg_Request>
request_;
I will kill these three member variables. Will change CreateRequest to return
scoped_ptr<ResourceHostMsg_Request>

https://codereview.chromium.org/226273005/diff/490001/content/child/resource_...
content/child/resource_dispatcher.h:222: int request_id_;
I should also remove this, I'm not using it anymore.

https://codereview.chromium.org/226273005/diff/490001/content/child/resource_...
content/child/resource_dispatcher.h:225: GURL frame_origin_;
Not sure how I will fix this, but I will try to find a way to share it between
StartSync/StartAsync. It may be possible to just return it as an output
parameter from CreateRequest.

[tfarina, 2014-04-18 15:24:32]

John, could you please, review this again?

Thanks in advance.

https://codereview.chromium.org/226273005/diff/490001/content/child/resource_...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/490001/content/child/resource_...
content/child/resource_dispatcher.h:219: scoped_ptr<ResourceHostMsg_Request>
request_;
On 2014/04/18 02:31:45, tfarina wrote:
> I will kill these three member variables. Will change CreateRequest to return
> scoped_ptr<ResourceHostMsg_Request>

Done.

https://codereview.chromium.org/226273005/diff/490001/content/child/resource_...
content/child/resource_dispatcher.h:222: int request_id_;
On 2014/04/18 02:31:45, tfarina wrote:
> I should also remove this, I'm not using it anymore.

Done.

https://codereview.chromium.org/226273005/diff/490001/content/child/resource_...
content/child/resource_dispatcher.h:225: GURL frame_origin_;
On 2014/04/18 02:31:45, tfarina wrote:
> Not sure how I will fix this, but I will try to find a way to share it between
> StartSync/StartAsync. It may be possible to just return it as an output
> parameter from CreateRequest.

Done.

https://codereview.chromium.org/226273005/diff/530001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/226273005/diff/530001/content/child/resource_...
content/child/resource_dispatcher.cc:701: *frame_origin =
extra_data->frame_origin();
John, any idea why frame_origin() call is crashing?

[jam, 2014-04-22 16:47:50]

https://codereview.chromium.org/226273005/diff/530001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/226273005/diff/530001/content/child/resource_...
content/child/resource_dispatcher.cc:701: *frame_origin =
extra_data->frame_origin();
On 2014/04/18 15:24:33, tfarina wrote:
> John, any idea why frame_origin() call is crashing?

it's crashing because extra_data points to an object that was allocated on the
stack and which is now gone.

ResourceDispatcherTest::CreateRequestInfo used to pass RequestExtraData
extra_data; to a method in the scope where it's created, but now it returns a
pointer to an object on the stack, which when it returns is gone.

[tfarina, 2014-04-22 23:46:49]

https://codereview.chromium.org/226273005/diff/530001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/226273005/diff/530001/content/child/resource_...
content/child/resource_dispatcher.cc:701: *frame_origin =
extra_data->frame_origin();
On 2014/04/22 16:47:51, jam wrote:
> On 2014/04/18 15:24:33, tfarina wrote:
> > John, any idea why frame_origin() call is crashing?
> 
> it's crashing because extra_data points to an object that was allocated on the
> stack and which is now gone.
> 
> ResourceDispatcherTest::CreateRequestInfo used to pass RequestExtraData
> extra_data; to a method in the scope where it's created, but now it returns a
> pointer to an object on the stack, which when it returns is gone.

John, based on your analysis, I was able to fix the crash I was getting in the
unittests. Thanks so much for spotting it! Does kEmptyData represents here also
a problem?

[tfarina, 2014-04-23 00:05:49]

I think my changes in ResourceDispatcher are triggering this:

[11281:11281:0422/210413:432962474428:FATAL:ref_counted.h(60)] Check failed:
!in_dtor_. 
#0 0x7f643f404a3e base::debug::StackTrace::StackTrace()
#1 0x7f643f48e5f5 logging::LogMessage::~LogMessage()
#2 0x7f6435d7bcc3 base::subtle::RefCountedBase::Release()
#3 0x7f64367b3c7f base::RefCounted<>::Release()
#4 0x7f64367b2837 content::WebURLLoaderImpl::Context::OnCompletedRequest()
#5 0x7f64367827e7 content::ResourceDispatcher::OnRequestComplete()
#6 0x7f6436785b91 DispatchToMethod<>()
#7 0x7f6436785126 ResourceMsg_RequestComplete::Dispatch<>()
#8 0x7f6436780a6f content::ResourceDispatcher::DispatchMessage()
#9 0x7f643677feed content::ResourceDispatcher::OnMessageReceived()
#10 0x7f64366bd7ab content::ChildThread::OnMessageReceived()
#11 0x7f643e0ba103 IPC::ChannelProxy::Context::OnDispatchMessage()
#12 0x7f643e0c215a base::internal::RunnableAdapter<>::Run()
#13 0x7f643e0c20b1 base::internal::InvokeHelper<>::MakeItSo()
#14 0x7f643e0c204c base::internal::Invoker<>::Run()
#15 0x7f643f3ec9fe base::Callback<>::Run()
#16 0x7f643f4b3ac8 base::MessageLoop::RunTask()
#17 0x7f643f4b3deb base::MessageLoop::DeferOrRunPendingTask()
#18 0x7f643f4b4015 base::MessageLoop::DoWork()
#19 0x7f643f4c4488 base::MessagePumpDefault::Run()
#20 0x7f643f4b3400 base::MessageLoop::RunHandler()
#21 0x7f643f519572 base::RunLoop::Run()
#22 0x7f643f4b2ca1 base::MessageLoop::Run()
#23 0x7f6436d5c691 content::RendererMain()
#24 0x7f6435d3238d content::RunZygote()
#25 0x7f6435d326a9 content::RunNamedProcessTypeMain()
#26 0x7f6435d344a8 content::ContentMainRunnerImpl::Run()
#27 0x7f6435d31c75 content::ContentMain()
#28 0x000000453ccc main
#29 0x7f643391776d __libc_start_main
#30 0x000000453b9d <unknown>

[jam, 2014-04-23 22:24:58]

On 2014/04/23 00:05:49, tfarina wrote:
> I think my changes in ResourceDispatcher are triggering this:
> 
> [11281:11281:0422/210413:432962474428:FATAL:ref_counted.h(60)] Check failed:
> !in_dtor_. 
> #0 0x7f643f404a3e base::debug::StackTrace::StackTrace()
> #1 0x7f643f48e5f5 logging::LogMessage::~LogMessage()
> #2 0x7f6435d7bcc3 base::subtle::RefCountedBase::Release()
> #3 0x7f64367b3c7f base::RefCounted<>::Release()
> #4 0x7f64367b2837 content::WebURLLoaderImpl::Context::OnCompletedRequest()
> #5 0x7f64367827e7 content::ResourceDispatcher::OnRequestComplete()
> #6 0x7f6436785b91 DispatchToMethod<>()
> #7 0x7f6436785126 ResourceMsg_RequestComplete::Dispatch<>()
> #8 0x7f6436780a6f content::ResourceDispatcher::DispatchMessage()
> #9 0x7f643677feed content::ResourceDispatcher::OnMessageReceived()
> #10 0x7f64366bd7ab content::ChildThread::OnMessageReceived()
> #11 0x7f643e0ba103 IPC::ChannelProxy::Context::OnDispatchMessage()
> #12 0x7f643e0c215a base::internal::RunnableAdapter<>::Run()
> #13 0x7f643e0c20b1 base::internal::InvokeHelper<>::MakeItSo()
> #14 0x7f643e0c204c base::internal::Invoker<>::Run()
> #15 0x7f643f3ec9fe base::Callback<>::Run()
> #16 0x7f643f4b3ac8 base::MessageLoop::RunTask()
> #17 0x7f643f4b3deb base::MessageLoop::DeferOrRunPendingTask()
> #18 0x7f643f4b4015 base::MessageLoop::DoWork()
> #19 0x7f643f4c4488 base::MessagePumpDefault::Run()
> #20 0x7f643f4b3400 base::MessageLoop::RunHandler()
> #21 0x7f643f519572 base::RunLoop::Run()
> #22 0x7f643f4b2ca1 base::MessageLoop::Run()
> #23 0x7f6436d5c691 content::RendererMain()
> #24 0x7f6435d3238d content::RunZygote()
> #25 0x7f6435d326a9 content::RunNamedProcessTypeMain()
> #26 0x7f6435d344a8 content::ContentMainRunnerImpl::Run()
> #27 0x7f6435d31c75 content::ContentMain()
> #28 0x000000453ccc main
> #29 0x7f643391776d __libc_start_main
> #30 0x000000453b9d <unknown>

This is happening because the object is being deleted twice. This is because in
WebURLLoaderImpl::Context::Start the AddRef() call was removed, so the matching
Release call in OnCompletedRequest deletes an object that is already deleted.
Put back the AddRef as before and it should work.


On 2014/04/22 23:46:49, tfarina wrote:
>
https://codereview.chromium.org/226273005/diff/530001/content/child/resource_...
> File content/child/resource_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/226273005/diff/530001/content/child/resource_...
> content/child/resource_dispatcher.cc:701: *frame_origin =
> extra_data->frame_origin();
> On 2014/04/22 16:47:51, jam wrote:
> > On 2014/04/18 15:24:33, tfarina wrote:
> > > John, any idea why frame_origin() call is crashing?
> > 
> > it's crashing because extra_data points to an object that was allocated on
the
> > stack and which is now gone.
> > 
> > ResourceDispatcherTest::CreateRequestInfo used to pass RequestExtraData
> > extra_data; to a method in the scope where it's created, but now it returns
a
> > pointer to an object on the stack, which when it returns is gone.
> 
> John, based on your analysis, I was able to fix the crash I was getting in the
> unittests. Thanks so much for spotting it! Does kEmptyData represents here
also
> a problem?

no it shouldn't, since it's only used within the scope where it's declared.

[tfarina, 2014-04-24 00:17:37]

That was it John. Thanks a lot! Putting it back, made the renderer work again.

ptal.

https://codereview.chromium.org/226273005/diff/550001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/226273005/diff/550001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:304: 0 /*routing_id*/,
John, is this right?

[jam, 2014-04-24 16:14:08]

https://codereview.chromium.org/226273005/diff/550001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/226273005/diff/550001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:304: 0 /*routing_id*/,
On 2014/04/24 00:17:38, tfarina wrote:
> John, is this right?

since routing_id isn't used, it should be removed as a parameter from the method

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
File content/child/resource_dispatcher.cc (left):

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
content/child/resource_dispatcher.cc:168: new
ResourceHostMsg_ReleaseDownloadedFile(request_id_));
hmm, this isn't called either, it'll have to be called just like
RemovePendingRequest.

there'll be some duplication in having two call these two methods in both the
pluginfetcher and weburlloader, however the former will go away near the end of
this year when we remove support for npapi

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
content/child/resource_dispatcher.cc:358: int
ResourceDispatcher::AddPendingRequest(RequestPeer* callback,
remove this method from the header and just inline the definition into the one
calling site in this file now

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
content/child/resource_dispatcher.h:82: bool RemovePendingRequest(int
request_id);
i just noticed this isn't called anymore. this will need to be called by
PluginURLFetcher and WebURLLoaderImpl's destructors.

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
content/child/resource_dispatcher.h:85: void CancelPendingRequest(int
request_id);
nit: rename this method to Cancel() now to match the other methods below which
also act on pending requests (hence the request_id parameter) but that don't
have the redundant PendingRequest suffix

[tfarina, 2014-04-26 04:23:49]

https://codereview.chromium.org/226273005/diff/550001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/226273005/diff/550001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:304: 0 /*routing_id*/,
On 2014/04/24 16:14:08, jam wrote:
> On 2014/04/24 00:17:38, tfarina wrote:
> > John, is this right?
> 
> since routing_id isn't used, it should be removed as a parameter from the
method

Done.

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
File content/child/resource_dispatcher.cc (left):

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
content/child/resource_dispatcher.cc:168: new
ResourceHostMsg_ReleaseDownloadedFile(request_id_));
On 2014/04/24 16:14:08, jam wrote:
> hmm, this isn't called either, it'll have to be called just like
> RemovePendingRequest.
> 
> there'll be some duplication in having two call these two methods in both the
> pluginfetcher and weburlloader, however the former will go away near the end
of
> this year when we remove support for npapi

Done.

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
content/child/resource_dispatcher.cc:358: int
ResourceDispatcher::AddPendingRequest(RequestPeer* callback,
On 2014/04/24 16:14:08, jam wrote:
> remove this method from the header and just inline the definition into the one
> calling site in this file now

Done.

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
content/child/resource_dispatcher.h:82: bool RemovePendingRequest(int
request_id);
On 2014/04/24 16:14:08, jam wrote:
> i just noticed this isn't called anymore. this will need to be called by
> PluginURLFetcher and WebURLLoaderImpl's destructors.

Done.

https://codereview.chromium.org/226273005/diff/570001/content/child/resource_...
content/child/resource_dispatcher.h:85: void CancelPendingRequest(int
request_id);
On 2014/04/24 16:14:08, jam wrote:
> nit: rename this method to Cancel() now to match the other methods below which
> also act on pending requests (hence the request_id parameter) but that don't
> have the redundant PendingRequest suffix

Done.

[jam, 2014-04-28 15:26:12]

Thanks, lgtm with this one comment

https://codereview.chromium.org/226273005/diff/590001/content/child/resource_...
File content/child/resource_dispatcher.h (right):

https://codereview.chromium.org/226273005/diff/590001/content/child/resource_...
content/child/resource_dispatcher.h:98: IPC::Sender* message_sender() const {
return message_sender_; }
nit: hide this once you do my other comment (sending the IPC in
RemovePendingRequest)

https://codereview.chromium.org/226273005/diff/590001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/226273005/diff/590001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:642: new
ResourceHostMsg_ReleaseDownloadedFile(request_id_));
nit: we should keep the sending of resource related IPCs in ResourceDispatcher.

How about make this callsite call RemovePendingRequest as it does above. and in
that method, it can do this check of downloadToFile

[tfarina, 2014-04-30 01:33:10]

https://codereview.chromium.org/226273005/diff/590001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/226273005/diff/590001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:283:
ChildThread::current()->resource_dispatcher()->Cancel(request_id_);
John, this is crashing:

[ RUN      ] RenderViewImplTest.OnHandleKeyboardEvent
[1635:1635:0429/222921:519387653807:WARNING:resource_bundle.cc(283)]
locale_file_path.empty()
Received signal 11 SEGV_MAPERR 000000000138
#0 0x7fc7345bfe7e base::debug::StackTrace::StackTrace()
#1 0x7fc7345bf9b7 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fc72713bcb0 <unknown>
#3 0x7fc72b46c01c base::internal::scoped_ptr_impl<>::get()
#4 0x7fc72b46d7b5 scoped_ptr<>::get()
#5 0x7fc72b46ae1c content::ChildThread::resource_dispatcher()
#6 0x7fc72b55e881 content::WebURLLoaderImpl::Context::Cancel()
#7 0x7fc72b562864 content::WebURLLoaderImpl::cancel()
#8 0x7fc7305f71a5 WebCore::ResourceLoader::releaseResources()
#9 0x7fc7305f92dc WebCore::ResourceLoader::didFinishLoading()
#10 0x7fc72b561c8d content::WebURLLoaderImpl::Context::OnCompletedRequest()
#11 0x7fc72b55fef5 content::WebURLLoaderImpl::Context::HandleDataURL()

I tried adding:
 if (ChildThread::current()->resource_dispatcher()) 

but that didn't help.

https://codereview.chromium.org/226273005/diff/590001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:642: new
ResourceHostMsg_ReleaseDownloadedFile(request_id_));
On 2014/04/28 15:26:13, jam wrote:
> nit: we should keep the sending of resource related IPCs in
ResourceDispatcher.
> 
> How about make this callsite call RemovePendingRequest as it does above. and
in
> that method, it can do this check of downloadToFile
John, ResourceDispatcher does not have |request_| to access downloadToFile. Do
you want me to store this bool in PendingRequestInfo struct? I think I get get
it from RequestInfo::download_to_file.

[tfarina, 2014-05-01 03:59:46]

Ping?

[jam, 2014-05-01 16:28:15]

https://codereview.chromium.org/226273005/diff/590001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/226273005/diff/590001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:283:
ChildThread::current()->resource_dispatcher()->Cancel(request_id_);
On 2014/04/30 01:33:11, tfarina wrote:
> John, this is crashing:
> 
> [ RUN      ] RenderViewImplTest.OnHandleKeyboardEvent
> [1635:1635:0429/222921:519387653807:WARNING:resource_bundle.cc(283)]
> locale_file_path.empty()
> Received signal 11 SEGV_MAPERR 000000000138
> #0 0x7fc7345bfe7e base::debug::StackTrace::StackTrace()
> #1 0x7fc7345bf9b7 base::debug::(anonymous namespace)::StackDumpSignalHandler()
> #2 0x7fc72713bcb0 <unknown>
> #3 0x7fc72b46c01c base::internal::scoped_ptr_impl<>::get()
> #4 0x7fc72b46d7b5 scoped_ptr<>::get()
> #5 0x7fc72b46ae1c content::ChildThread::resource_dispatcher()
> #6 0x7fc72b55e881 content::WebURLLoaderImpl::Context::Cancel()
> #7 0x7fc72b562864 content::WebURLLoaderImpl::cancel()
> #8 0x7fc7305f71a5 WebCore::ResourceLoader::releaseResources()
> #9 0x7fc7305f92dc WebCore::ResourceLoader::didFinishLoading()
> #10 0x7fc72b561c8d content::WebURLLoaderImpl::Context::OnCompletedRequest()
> #11 0x7fc72b55fef5 content::WebURLLoaderImpl::Context::HandleDataURL()
> 
> I tried adding:
>  if (ChildThread::current()->resource_dispatcher()) 
> 
> but that didn't help.
> 

you probably need to check for

if (ChildThread::current())  // NULL in unittests

instead

https://codereview.chromium.org/226273005/diff/590001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:642: new
ResourceHostMsg_ReleaseDownloadedFile(request_id_));
On 2014/04/30 01:33:11, tfarina wrote:
> On 2014/04/28 15:26:13, jam wrote:
> > nit: we should keep the sending of resource related IPCs in
> ResourceDispatcher.
> > 
> > How about make this callsite call RemovePendingRequest as it does above. and
> in
> > that method, it can do this check of downloadToFile
> John, ResourceDispatcher does not have |request_| to access downloadToFile. Do
> you want me to store this bool in PendingRequestInfo struct? I think I get get
> it from RequestInfo::download_to_file.

sure

[tfarina, 2014-05-01 19:57:15]

ptal

https://codereview.chromium.org/226273005/diff/590001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/226273005/diff/590001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:283:
ChildThread::current()->resource_dispatcher()->Cancel(request_id_);
On 2014/05/01 16:28:16, jam wrote:
> On 2014/04/30 01:33:11, tfarina wrote:
> > John, this is crashing:
> > 
> > [ RUN      ] RenderViewImplTest.OnHandleKeyboardEvent
> > [1635:1635:0429/222921:519387653807:WARNING:resource_bundle.cc(283)]
> > locale_file_path.empty()
> > Received signal 11 SEGV_MAPERR 000000000138
> > #0 0x7fc7345bfe7e base::debug::StackTrace::StackTrace()
> > #1 0x7fc7345bf9b7 base::debug::(anonymous
namespace)::StackDumpSignalHandler()
> > #2 0x7fc72713bcb0 <unknown>
> > #3 0x7fc72b46c01c base::internal::scoped_ptr_impl<>::get()
> > #4 0x7fc72b46d7b5 scoped_ptr<>::get()
> > #5 0x7fc72b46ae1c content::ChildThread::resource_dispatcher()
> > #6 0x7fc72b55e881 content::WebURLLoaderImpl::Context::Cancel()
> > #7 0x7fc72b562864 content::WebURLLoaderImpl::cancel()
> > #8 0x7fc7305f71a5 WebCore::ResourceLoader::releaseResources()
> > #9 0x7fc7305f92dc WebCore::ResourceLoader::didFinishLoading()
> > #10 0x7fc72b561c8d content::WebURLLoaderImpl::Context::OnCompletedRequest()
> > #11 0x7fc72b55fef5 content::WebURLLoaderImpl::Context::HandleDataURL()
> > 
> > I tried adding:
> >  if (ChildThread::current()->resource_dispatcher()) 
> > 
> > but that didn't help.
> > 
> 
> you probably need to check for
> 
> if (ChildThread::current())  // NULL in unittests
> 
> instead

Fixed, it does not crash now, but the test failed (I hope is just an issue
locally and in the trybots it will be fine). Done.

https://codereview.chromium.org/226273005/diff/590001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:642: new
ResourceHostMsg_ReleaseDownloadedFile(request_id_));
On 2014/05/01 16:28:16, jam wrote:
> On 2014/04/30 01:33:11, tfarina wrote:
> > On 2014/04/28 15:26:13, jam wrote:
> > > nit: we should keep the sending of resource related IPCs in
> > ResourceDispatcher.
> > > 
> > > How about make this callsite call RemovePendingRequest as it does above.
and
> > in
> > > that method, it can do this check of downloadToFile
> > John, ResourceDispatcher does not have |request_| to access downloadToFile.
Do
> > you want me to store this bool in PendingRequestInfo struct? I think I get
get
> > it from RequestInfo::download_to_file.
> 
> sure

Done.

[tfarina, 2014-05-02 21:12:06]

John, I'm pushing this to CQ with your previous approval.

The bots are green now.

I can address any further changes you want in a follow up.

[tfarina, 2014-05-02 21:12:47]

TBRing Darin for the webkit/ changes.

[tfarina, 2014-05-02 21:12:59]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-05-02 21:13:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tfarina@chromium.org/226273005/630001

[commit-bot: I haz the power, 2014-05-02 21:35:30]

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

[commit-bot: I haz the power, 2014-05-02 21:35:30]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium

[tfarina, 2014-05-02 21:52:03]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-05-02 21:52:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tfarina@chromium.org/226273005/630001

[commit-bot: I haz the power, 2014-05-02 22:40:53]

Change committed as 267947

[DaleCurtis, 2014-05-03 01:50:00]

A revert of this CL has been created in
https://codereview.chromium.org/267973002/ by dalecurtis@chromium.org.

The reason for reverting is: Looks like it broke LSan bots:

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
.