Properly handle defers loading in WebURLLoaderImpl for data uris

Properly handle defers loading in WebURLLoaderImpl for data uris

BUG=418116
Committed: https://crrev.com/7f6143991b4b1b6024427d885ab90a665798eed1
Cr-Commit-Position: refs/heads/master@{#306274}

[João Eiras, 2014-11-18 15:12:46]

joaoe@opera.com changed reviewers:
+ kbr@chromium.org, tkent@chromium.org

[João Eiras, 2014-11-18 15:12:47]

This fix is related to https://codereview.chromium.org/625583002/ which includes
some tests

[João Eiras, 2014-11-18 15:16:18]

joaoe@opera.com changed reviewers:
+ sievers@chromium.org, skyostil@google.com
- kbr@chromium.org, tkent@chromium.org

[Ken Russell (switch to Gerrit), 2014-11-18 20:41:26]

kbr@chromium.org changed reviewers:
+ davidben@chromium.org, jochen@chromium.org, kbr@chromium.org,
mkwst@chromium.org

[Ken Russell (switch to Gerrit), 2014-11-18 20:41:26]

CC'ing folks more likely to be good for reviewing this change.

[davidben, 2014-11-18 21:02:30]

This is probably worth adding tests for. I see some data URL tests in
web_url_loader_impl_unittest.cc that probably could be adapted here. Thanks!

I'd also add a bit more context to the commit message for why this is needed for
crbug.com/418116. If I understand you correctly, this is for some tests in
Blink?

Otherwise, just some minor nitpicks.

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:357: enum {NO_DEFER, DEFERS, DEFERED_DATA}
defers_loading_;
Nit: I ended up pausing a bit to check whether we usually define enums that way.
Maybe give it a name like enum DeferState or so. (defer_state_ is probably a
better name too? defers_loading_ sounds like it should be a bool.)

Other nit: deferred has two r's. :-)

Other other nit: the three enum names don't really match in tense very well.
Maybe NOT_DEFERRING, DEFERRING, DEFERRED_DATA?

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:397: DCHECK_EQ(defers_loading_, NO_DEFER);
Nit: I'd do DCHECK_EQ(NO_DEFER, defers_loading_) to be consistent with EXPECT_EQ
and ASSERT_EQ. (For the latter, the expected value is on the left.)

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:401: if (defers_loading_ == DEFERED_DATA)
Nit: curly braces since the body is multiple lines.

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:402:
base::MessageLoop::current()->PostTask(
I believe base::MessageLoop::current() should be task_runner_

[jochen (gone - plz use gerrit), 2014-11-19 08:54:06]

can you add a unit test please?

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:357: enum {NO_DEFER, DEFERS, DEFERED_DATA}
defers_loading_;
it also doesn't look clang-formatted.

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:852: if (defers_loading_ != NO_DEFER) {
deferes_loading_ should be "DEFERS" if it's != NO_DEFER here, right?

[João Eiras, 2014-11-21 15:25:32]

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:357: enum {NO_DEFER, DEFERS, DEFERED_DATA}
defers_loading_;
On 2014/11/18 21:02:29, David Benjamin wrote:
> Nit: I ended up pausing a bit to check whether we usually define enums that
way.
> Maybe give it a name like enum DeferState or so. (defer_state_ is probably a
> better name too? defers_loading_ sounds like it should be a bool.)
> 
> Other nit: deferred has two r's. :-)
> 
> Other other nit: the three enum names don't really match in tense very well.
> Maybe NOT_DEFERRING, DEFERRING, DEFERRED_DATA?

Done. (I did find deferring and deferred_data to sound very ambiguous).

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:397: DCHECK_EQ(defers_loading_, NO_DEFER);
On 2014/11/18 21:02:29, David Benjamin wrote:
> Nit: I'd do DCHECK_EQ(NO_DEFER, defers_loading_) to be consistent with
EXPECT_EQ
> and ASSERT_EQ. (For the latter, the expected value is on the left.)

done

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:401: if (defers_loading_ == DEFERED_DATA)
On 2014/11/18 21:02:29, David Benjamin wrote:
> Nit: curly braces since the body is multiple lines.

done

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:402:
base::MessageLoop::current()->PostTask(
On 2014/11/18 21:02:29, David Benjamin wrote:
> I believe base::MessageLoop::current() should be task_runner_

done

https://codereview.chromium.org/737763002/diff/1/content/child/web_url_loader...
content/child/web_url_loader_impl.cc:852: if (defers_loading_ != NO_DEFER) {
On 2014/11/19 08:54:06, jochen (slow) wrote:
> deferes_loading_ should be "DEFERS" if it's != NO_DEFER here, right?

It should. Added an assert.

[davidben, 2014-11-21 18:34:00]

lgtm with a comment on the test.

https://codereview.chromium.org/737763002/diff/20001/content/child/web_url_lo...
File content/child/web_url_loader_impl_unittest.cc (right):

https://codereview.chromium.org/737763002/diff/20001/content/child/web_url_lo...
content/child/web_url_loader_impl_unittest.cc:533:
EXPECT_FALSE(client()->did_finish());
Why do this twice? I would have thought that a RunUntilIdle() couldn't do
anything, unless there's something flaking between threads. (Assuming
did_finish() can't PostTask because that would be kinda crazy.)

[João Eiras, 2014-11-24 13:25:41]

On 2014/11/18 21:02:30, David Benjamin wrote:
> This is probably worth adding tests for. I see some data URL tests in
> web_url_loader_impl_unittest.cc that probably could be adapted here. Thanks!
> 
> I'd also add a bit more context to the commit message for why this is needed
for
> crbug.com/418116. If I understand you correctly, this is for some tests in
> Blink?
> 

(Sorry, forgot to answer).

This is part of the bug fix, which is triggered in two different parts of the
code.
This fix solves the assert "!mainResourceLoader() ||
!mainResourceLoader()->defersLoading()" specifically.

[jochen (gone - plz use gerrit), 2014-11-25 12:33:18]

lgtm

[João Eiras, 2014-12-01 21:30:13]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2014-12-01 21:30:45]

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

[commit-bot: I haz the power, 2014-12-01 22:39:45]

Committed patchset #3 (id:20002)

[commit-bot: I haz the power, 2014-12-01 22:40:32]

Patchset 3 (id:??) landed as
https://crrev.com/7f6143991b4b1b6024427d885ab90a665798eed1
Cr-Commit-Position: refs/heads/master@{#306274}

[Ken Russell (switch to Gerrit), 2014-12-02 01:28:42]

A revert of this CL (patchset #3 id:20002) has been created in
https://codereview.chromium.org/766963003/ by kbr@chromium.org.

The reason for reverting is: Caused assertion failures in Blink layout tests.
See:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20%28db...

Stack trace (I can't add this to the bug, the issue tracker is broken right
now):

crash log for renderer (pid 4510):
STDOUT: <empty>
STDERR: [4510:263:1201/154140:461606007011:FATAL:web_url_loader_impl.cc(401)]
Check failed: NOT_DEFERRING != defers_loading_ (0 vs. 0)
STDERR: 0   libbase.dylib                       0x00000001e3133dcf
base::debug::StackTrace::StackTrace() + 47
STDERR: 1   libbase.dylib                       0x00000001e3133e23
base::debug::StackTrace::StackTrace() + 35
STDERR: 2   libbase.dylib                       0x00000001e31d3356
logging::LogMessage::~LogMessage() + 70
STDERR: 3   libbase.dylib                       0x00000001e31d23a3
logging::LogMessage::~LogMessage() + 35
STDERR: 4   libcontent.dylib                    0x00000001f1809222
content::WebURLLoaderImpl::Context::SetDefersLoading(bool) + 450
STDERR: 5   libcontent.dylib                    0x00000001f180f346
content::WebURLLoaderImpl::setDefersLoading(bool) + 70
STDERR: 6   libblink_web.dylib                  0x00000001e9aff51d
blink::ResourceLoader::setDefersLoading(bool) + 157
STDERR: 7   libblink_web.dylib                  0x00000001e9ac2168
blink::RawResource::setDefersLoading(bool) + 104
STDERR: 8   libblink_web.dylib                  0x00000001e9ddd5d8
blink::DocumentThreadableLoader::setDefersLoading(bool) + 104
STDERR: 9   libblink_web.dylib                  0x00000001e9d2316a
blink::InspectorResourceAgent::loadResourceForFrontend(WTF::String*, WTF::String
const&, WTF::String const&, WTF::RefPtr<blink::JSONObject> const*,
WTF::PassRefPtr<blink::InspectorBackendDispatcher::NetworkCommandHandler::LoadResourceForFrontendCallback>)
+ 1306
STDERR: 10  libblink_web.dylib                  0x00000001e9d233fc non-virtual
thunk to blink::InspectorResourceAgent::loadResourceForFrontend(WTF::String*,
WTF::String const&, WTF::String const&, WTF::RefPtr<blink::JSONObject> const*,
WTF::PassRefPtr<blink::InspectorBackendDispatcher::NetworkCommandHandler::LoadResourceForFrontendCallback>)
+ 76
STDERR: 11  libblink_web.dylib                  0x00000001e8c09a8a
blink::InspectorBackendDispatcherImpl::Network_loadResourceForFrontend(long,
blink::JSONObject*, blink::JSONArray*) + 810
STDERR: 12  libblink_web.dylib                  0x00000001e8c2cce6
blink::InspectorBackendDispatcherImpl::dispatch(WTF::String const&) + 1830
STDERR: 13  libblink_web.dylib                  0x00000001e9c72a61
blink::InspectorController::dispatchMessageFromFrontend(WTF::String const&) +
113
STDERR: 14  libblink_web.dylib                  0x00000001e770b6a5
blink::WebDevToolsAgentImpl::dispatchOnInspectorBackend(blink::WebString const&)
+ 69
STDERR: 15  libcontent.dylib                    0x00000001f1c4afe7
content::DevToolsAgent::OnDispatchOnInspectorBackend(std::string const&) + 327
STDERR: 16  libcontent.dylib                    0x00000001f1c4e4f5 void
DispatchToMethod<content::DevToolsAgent, void
(content::DevToolsAgent::*)(std::string const&),
std::string>(content::DevToolsAgent*, void
(content::DevToolsAgent::*)(std::string const&), Tuple1<std::string> const&) +
165
STDERR: 17  libcontent.dylib                    0x00000001f1c4d2c5 bool
DevToolsAgentMsg_DispatchOnInspectorBackend::Dispatch<content::DevToolsAgent,
content::DevToolsAgent, void, void (content::DevToolsAgent::*)(std::string
const&)>(IPC::Message const*, content::DevToolsAgent*, content::DevToolsAgent*,
void*, void (content::DevToolsAgent::*)(std::string const&)) + 133
STDERR: 18  libcontent.dylib                    0x00000001f1c4a915
content::DevToolsAgent::OnMessageReceived(IPC::Message const&) + 1205
STDERR: 19  libcontent.dylib                    0x00000001f1d94d71
content::RenderFrameImpl::OnMessageReceived(IPC::Message const&) + 401
STDERR: 20  libcontent.dylib                    0x00000001f1bd022e
content::MessageRouter::RouteMessage(IPC::Message const&) + 110
STDERR: 21  libcontent.dylib                    0x00000001f1bd018c
content::MessageRouter::OnMessageReceived(IPC::Message const&) + 108
STDERR: 22  libcontent.dylib                    0x00000001f16964ef
content::ChildThread::OnMessageReceived(IPC::Message const&) + 1951
STDERR: 23  libipc.dylib                        0x00000001f87cd844
IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const&) + 468
STDERR: 24  libipc.dylib                        0x00000001f87d6e86
base::internal::RunnableAdapter<void
(IPC::ChannelProxy::Context::*)(IPC::Message
const&)>::Run(IPC::ChannelProxy::Context*, IPC::Message const&) + 150
STDERR: 25  libipc.dylib                        0x00000001f87d6d8f
base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void
(IPC::ChannelProxy::Context::*)(IPC::Message const&)>, void
()(IPC::ChannelProxy::Context* const&, IPC::Message
const&)>::MakeItSo(base::internal::RunnableAdapter<void
(IPC::ChannelProxy::Context::*)(IPC::Message const&)>,
IPC::ChannelProxy::Context* const&, IPC::Message const&) + 79
STDERR: 26  libipc.dylib                        0x00000001f87d6cdc
base::internal::Invoker<2,
base::internal::BindState<base::internal::RunnableAdapter<void
(IPC::ChannelProxy::Context::*)(IPC::Message const&)>, void
()(IPC::ChannelProxy::Context*, IPC::Message const&), void
()(IPC::ChannelProxy::Context*, IPC::Message)>, void
()(IPC::ChannelProxy::Context*, IPC::Message
const&)>::Run(base::internal::BindStateBase*) + 140
STDERR: 27  libbase.dylib                       0x00000001e311ea6f
base::Callback<void ()()>::Run() const + 63
STDERR: 28  libbase.dylib                       0x00000001e3135fc1
base::debug::TaskAnnotator::RunTask(char const*, char const*, base::PendingTask
const&) + 977
STDERR: 29  libbase.dylib                       0x00000001e3217230
base::MessageLoop::RunTask(base::PendingTask const&) + 640
STDERR: 30  libbase.dylib                       0x00000001e32173b9
base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) + 89
STDERR: 31  libbase.dylib                       0x00000001e3217683
base::MessageLoop::DoWork() + 323
STDERR: 32  libbase.dylib                       0x00000001e30f968b
base::MessagePumpCFRunLoopBase::RunWork() + 107
STDERR: 33  libbase.dylib                       0x00000001e30f8b3b
base::MessagePumpCFRunLoopBase::RunWorkSource(void*) + 43
STDERR: 34  CoreFoundation                      0x00007fff89d203d1
__CFRunLoopDoSources0 + 1361
STDERR: 35  CoreFoundation                      0x00007fff89d1e5c9
__CFRunLoopRun + 873
STDERR: 36  CoreFoundation                      0x00007fff89d1dd8f
CFRunLoopRunSpecific + 575
STDERR: 37  Foundation                          0x00007fff80443b74
-[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 270
STDERR: 38  libbase.dylib                       0x00000001e30fa277
base::MessagePumpNSRunLoop::DoRun(base::MessagePump::Delegate*) + 151
STDERR: 39  libbase.dylib                       0x00000001e30f92e0
base::MessagePumpCFRunLoopBase::Run(base::MessagePump::Delegate*) + 128
STDERR: 40  libbase.dylib                       0x00000001e3216cd9
base::MessageLoop::RunHandler() + 249
STDERR: 41  libbase.dylib                       0x00000001e327e945
base::RunLoop::Run() + 85
STDERR: 42  libbase.dylib                       0x00000001e321624f
base::MessageLoop::Run() + 47
STDERR: 43  libcontent.dylib                    0x00000001f1e66368
content::RendererMain(content::MainFunctionParams const&) + 1608
STDERR: 44  libcontent.dylib                    0x00000001f096e920
content::RunNamedProcessTypeMain(std::string const&, content::MainFunctionParams
const&, content::ContentMainDelegate*) + 256
STDERR: 45  libcontent.dylib                    0x00000001f096fcfe
content::ContentMainRunnerImpl::Run() + 622
STDERR: 46  libcontent.dylib                    0x00000001f096df73
content::ContentMain(content::ContentMainParams const&) + 147
STDERR: 47  Content Shell Framework             0x00000001df4e0893 ContentMain +
83
STDERR: 48  Content Shell Helper                0x00000001df4d9f00 main + 48
STDERR: 49  Content Shell Helper                0x00000001df4d9ec4 start + 52
.

[João Eiras, 2014-12-02 13:55:53]

David, Jochen and Ken, I made the unit test a bit more convoluted to test for
the kind of unorderly use the API was subjected to by the tests that failed.
Still lgtm ?

[Ken Russell (switch to Gerrit), 2014-12-02 19:47:06]

On 2014/12/02 13:55:53, João Eiras wrote:
> David, Jochen and Ken, I made the unit test a bit more convoluted to test for
> the kind of unorderly use the API was subjected to by the tests that failed.
> Still lgtm ?

Could you please create a new CL and point to this one, rather than re-landing
this again?

Did your changes fix the test failures on the Blink side?

[João Eiras, 2014-12-02 20:34:02]

On 2014/12/02 19:47:06, Ken Russell wrote:
> On 2014/12/02 13:55:53, João Eiras wrote:
> > David, Jochen and Ken, I made the unit test a bit more convoluted to test
for
> > the kind of unorderly use the API was subjected to by the tests that failed.
> > Still lgtm ?
> 
> Could you please create a new CL and point to this one, rather than re-landing
> this again?
> 

Sure

> Did your changes fix the test failures on the Blink side?

Yes. That was the point.

[Ken Russell (switch to Gerrit), 2014-12-03 00:05:31]

On 2014/12/02 20:34:02, João Eiras wrote:
> On 2014/12/02 19:47:06, Ken Russell wrote:
> > On 2014/12/02 13:55:53, João Eiras wrote:
> > > David, Jochen and Ken, I made the unit test a bit more convoluted to test
> for
> > > the kind of unorderly use the API was subjected to by the tests that
failed.
> > > Still lgtm ?
> > 
> > Could you please create a new CL and point to this one, rather than
re-landing
> > this again?
> > 
> 
> Sure

OK, thanks. I'm going to mark this as closed, assuming we'll review the new CL
instead.

> > Did your changes fix the test failures on the Blink side?
> 
> Yes. That was the point.

Great. Thanks for confirming.