[Mojo-Loading] Dispatch body data after response is received

[Mojo-Loading] Dispatch body data after response is received

MojoAsyncResourceHandler sometimes calls  OnStartLoadingResponseBody
before calling OnReceiveResponse. On the other hand, Blink doesn't
expect onDataRecieved is called before onReceiveResponse.

With this CL, URLLoaderClientImpl doesn't start reading the response body
until OnReceivedResponse arrives.

BUG=659917
Committed: https://crrev.com/17be6599bb1c41d82b1193ab24ca0173cbe897be
Cr-Commit-Position: refs/heads/master@{#429787}

[yhirano, 2016-10-27 09:37:40]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 09:38:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 10:34:01]

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

[commit-bot: I haz the power, 2016-10-27 10:34:02]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-10-27 11:04:48]

Description was changed from

==========
[Mojo-Loading] Dispatch body data after response is received

BUG=659917
==========

to

==========
[Mojo-Loading] Dispatch body data after response is received

MojoAsyncResourceHandler sometimes calls  OnStartLoadingResponseBody
before calling OnReceiveResponse. On the other hand, Blink doesn't
expect onDataRecieved is called before onReceiveResponse.

With this CL, URLLoaderClientImpl wait for OnReceivedResponse before
reading response body.

BUG=659917
==========

[yhirano, 2016-10-27 11:05:00]

Description was changed from

==========
[Mojo-Loading] Dispatch body data after response is received

MojoAsyncResourceHandler sometimes calls  OnStartLoadingResponseBody
before calling OnReceiveResponse. On the other hand, Blink doesn't
expect onDataRecieved is called before onReceiveResponse.

With this CL, URLLoaderClientImpl wait for OnReceivedResponse before
reading response body.

BUG=659917
==========

to

==========
[Mojo-Loading] Dispatch body data after response is received

MojoAsyncResourceHandler sometimes calls  OnStartLoadingResponseBody
before calling OnReceiveResponse. On the other hand, Blink doesn't
expect onDataRecieved is called before onReceiveResponse.

With this CL, URLLoaderClientImpl waits for OnReceivedResponse before
reading response body.

BUG=659917
==========

[yhirano, 2016-10-27 11:05:55]

Description was changed from

==========
[Mojo-Loading] Dispatch body data after response is received

MojoAsyncResourceHandler sometimes calls  OnStartLoadingResponseBody
before calling OnReceiveResponse. On the other hand, Blink doesn't
expect onDataRecieved is called before onReceiveResponse.

With this CL, URLLoaderClientImpl waits for OnReceivedResponse before
reading response body.

BUG=659917
==========

to

==========
[Mojo-Loading] Dispatch body data after response is received

MojoAsyncResourceHandler sometimes calls  OnStartLoadingResponseBody
before calling OnReceiveResponse. On the other hand, Blink doesn't
expect onDataRecieved is called before onReceiveResponse.

With this CL, URLLoaderClientImpl doesn't start reading the response body
until OnReceivedResponse arrives.

BUG=659917
==========

[yhirano, 2016-10-27 11:08:16]

yhirano@chromium.org changed reviewers:
+ tzik@chromium.org

[yhirano, 2016-10-27 11:08:16]

This CL is for deflaking (not yet added) XHR layout tests with mojo-loading.
I'll add them when they become stable enough.

[yhirano, 2016-10-31 04:51:40]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 04:51:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tzik, 2016-10-31 05:01:36]

lgtm

[commit-bot: I haz the power, 2016-10-31 05:32:49]

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

[commit-bot: I haz the power, 2016-10-31 05:32:53]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-10-31 10:47:20]

yhirano@chromium.org changed reviewers:
+ jam@chromium.org

[yhirano, 2016-10-31 10:47:21]

+jam@ for OWNER reivew.

[yhirano, 2016-11-03 13:07:24]

On 2016/10/31 10:47:21, yhirano wrote:
> +jam@ for OWNER reivew.

jam@, can you take a look?

[jam, 2016-11-04 00:45:31]

lgtm

[yhirano, 2016-11-04 01:09:58]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-11-04 01:10:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 05:03:34]

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

[commit-bot: I haz the power, 2016-11-04 05:03:45]

Failed to apply the patch.
On branch working_branch
Your branch is up-to-date with 'origin/refs/pending/heads/master'.
nothing to commit, working tree clean

[commit-bot: I haz the power, 2016-11-04 05:04:15]

Description was changed from

==========
[Mojo-Loading] Dispatch body data after response is received

MojoAsyncResourceHandler sometimes calls  OnStartLoadingResponseBody
before calling OnReceiveResponse. On the other hand, Blink doesn't
expect onDataRecieved is called before onReceiveResponse.

With this CL, URLLoaderClientImpl doesn't start reading the response body
until OnReceivedResponse arrives.

BUG=659917
==========

to

==========
[Mojo-Loading] Dispatch body data after response is received

MojoAsyncResourceHandler sometimes calls  OnStartLoadingResponseBody
before calling OnReceiveResponse. On the other hand, Blink doesn't
expect onDataRecieved is called before onReceiveResponse.

With this CL, URLLoaderClientImpl doesn't start reading the response body
until OnReceivedResponse arrives.

BUG=659917

Committed: https://crrev.com/17be6599bb1c41d82b1193ab24ca0173cbe897be
Cr-Commit-Position: refs/heads/master@{#429787}
==========

[commit-bot: I haz the power, 2016-11-04 05:04:15]

Patchset 2 (id:??) landed as
https://crrev.com/17be6599bb1c41d82b1193ab24ca0173cbe897be
Cr-Commit-Position: refs/heads/master@{#429787}

[tapted, 2016-11-04 08:32:43]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2466263008/ by tapted@chromium.org.

The reason for reverting is: Causing ASAN errors on WebKit Linux Precise Since
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precis...
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux_Pre...
errors like
virtual/mojo-loading/http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-content-type-header.html

==4==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e0000b75d1 at
pc 0x00000686e88c bp 0x7fffa32eeff0 sp 0x7fffa32eefe8
READ of size 1 at 0x60e0000b75d1 thread T0 (content_shell)
    #0 0x686e88b in OnReadable content/child/url_response_body_consumer.cc:91:11
    #1 0x46069ff in Run base/callback.h:64:12
    #2 0x46069ff in OnHandleReady mojo/public/cpp/system/watcher.cc:122:0
    #3 0x416c973 in Run base/callback.h:47:12
    /* snip */

0x60e0000b75d1 is located 145 bytes inside of 152-byte region
[0x60e0000b7540,0x60e0000b75d8)
freed by thread T0 (content_shell) here:
   ..
    #8 0xc782b14 in ~unique_ptr
buildtools/third_party/libc++/trunk/include/memory:2703:0
    #9 0xc782b14 in OnReceivedData content/child/web_url_loader_impl.cc:988:0
    #10 0x686e757 in OnReadable
content/child/url_response_body_consumer.cc:113:25
    #11 0x46069ff in Run base/callback.h:64:12
    #12 0x46069ff in OnHandleReady mojo/public/cpp/system/watcher.cc:122:0
    #13 0x416c973 in Run base/callback.h:47:12
    /* snip */.

[tapted, 2016-11-04 08:44:05]

tapted@chromium.org changed reviewers:
+ tapted@chromium.org

[tapted, 2016-11-04 08:44:06]

https://codereview.chromium.org/2459483002/diff/20001/content/child/url_respo...
File content/child/url_response_body_consumer.cc (right):

https://codereview.chromium.org/2459483002/diff/20001/content/child/url_respo...
content/child/url_response_body_consumer.cc:91: while (!has_been_cancelled_) {
So the UAF is actually down here, so https://codereview.chromium.org/2463753002/
(which started running the test) is probably the one that needs reverting. I
cancelled the revert of this CL.



==4==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e0000b75d1 at
pc 0x00000686e88c bp 0x7fffa32eeff0 sp 0x7fffa32eefe8
READ of size 1 at 0x60e0000b75d1 thread T0 (content_shell)
    #0 0x686e88b in OnReadable content/child/url_response_body_consumer.cc:91:11
    #1 0x46069ff in Run base/callback.h:64:12
    #2 0x46069ff in OnHandleReady mojo/public/cpp/system/watcher.cc:122:0
    #3 0x416c973 in Run base/callback.h:47:12
    #4 0x416c973 in RunTask base/debug/task_annotator.cc:52:0
    #5 0x71db744 in ProcessTaskFromWorkQueue
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:378:19
    #6 0x71d77c9 in DoWork
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:253:13
    #7 0x416c973 in Run base/callback.h:47:12
    #8 0x416c973 in RunTask base/debug/task_annotator.cc:52:0
    #9 0x403b960 in RunTask base/message_loop/message_loop.cc:413:19
    #10 0x403c255 in DeferOrRunPendingTask
base/message_loop/message_loop.cc:422:5
    #11 0x403d0de in DoWork base/message_loop/message_loop.cc:515:13
    #12 0x4045600 in Run base/message_loop/message_pump_default.cc:35:31
    #13 0x403b0d0 in RunHandler base/message_loop/message_loop.cc:378:10
    #14 0x40907e5 in Run base/run_loop.cc:35:10
    #15 0x6a4eabf in RendererMain content/renderer/renderer_main.cc:198:23
    #16 0x2eea179 in RunZygote content/app/content_main_runner.cc:336:14
    #17 0x2eecf19 in Run content/app/content_main_runner.cc:776:12
    #18 0x2ed8f5a in ContentMain content/app/content_main.cc:20:28
    #19 0x51831d in main content/shell/app/shell_main.cc:48:10
    #20 0x7f1b049c576c in __libc_start_main
/build/eglibc-rrybNj/eglibc-2.15/csu/libc-start.c:226:0

0x60e0000b75d1 is located 145 bytes inside of 152-byte region
[0x60e0000b7540,0x60e0000b75d8)
freed by thread T0 (content_shell) here:
    #0 0x515ceb in operator delete(void*) ??:0
    #1 0x686f7e2 in Release base/memory/ref_counted.h:135:7
    #2 0x686f7e2 in Release base/memory/ref_counted.h:406:0
    #3 0x686f7e2 in ~scoped_refptr base/memory/ref_counted.h:308:0
    #4 0x686f7e2 in ~ReceivedData
content/child/url_response_body_consumer.cc:25:0
    #5 0x686f7e2 in ~ReceivedData
content/child/url_response_body_consumer.cc:25:0
    #6 0xc782b14 in operator()
buildtools/third_party/libc++/trunk/include/memory:2529:13
    #7 0xc782b14 in reset
buildtools/third_party/libc++/trunk/include/memory:2735:0
    #8 0xc782b14 in ~unique_ptr
buildtools/third_party/libc++/trunk/include/memory:2703:0
    #9 0xc782b14 in OnReceivedData content/child/web_url_loader_impl.cc:988:0
    #10 0x686e757 in OnReadable
content/child/url_response_body_consumer.cc:113:25
    #11 0x46069ff in Run base/callback.h:64:12
    #12 0x46069ff in OnHandleReady mojo/public/cpp/system/watcher.cc:122:0
    #13 0x416c973 in Run base/callback.h:47:12
    #14 0x416c973 in RunTask base/debug/task_annotator.cc:52:0
    #15 0x71db744 in ProcessTaskFromWorkQueue
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:378:19
    #16 0x71d77c9 in DoWork
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:253:13
    #17 0x416c973 in Run base/callback.h:47:12
    #18 0x416c973 in RunTask base/debug/task_annotator.cc:52:0
    #19 0x403b960 in RunTask base/message_loop/message_loop.cc:413:19
    #20 0x403c255 in DeferOrRunPendingTask
base/message_loop/message_loop.cc:422:5
    #21 0x403d0de in DoWork base/message_loop/message_loop.cc:515:13
    #22 0x4045600 in Run base/message_loop/message_pump_default.cc:35:31
    #23 0x403b0d0 in RunHandler base/message_loop/message_loop.cc:378:10
    #24 0x40907e5 in Run base/run_loop.cc:35:10
    #25 0x6a4eabf in RendererMain content/renderer/renderer_main.cc:198:23
    #26 0x2eea179 in RunZygote content/app/content_main_runner.cc:336:14
    #27 0x2eecf19 in Run content/app/content_main_runner.cc:776:12
    #28 0x2ed8f5a in ContentMain content/app/content_main.cc:20:28
    #29 0x51831d in main content/shell/app/shell_main.cc:48:10
    #30 0x7f1b049c576c in __libc_start_main
/build/eglibc-rrybNj/eglibc-2.15/csu/libc-start.c:226:0

previously allocated by thread T0 (content_shell) here:
    #0 0x5150ab in operator new(unsigned long) ??:0
    #1 0x685b8ef in OnStartLoadingResponseBody
content/child/resource_dispatcher.cc:102:22
    #2 0x77973d in Accept
./out/Release/gen/content/common/url_loader.mojom.cc:396:13
    #3 0x45d56e8 in HandleValidatedMessage
mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:339:32
    #4 0x45e3b42 in ProcessIncomingMessage
mojo/public/cpp/bindings/lib/multiplex_router.cc:824:22
    #5 0x45e2d47 in Accept
mojo/public/cpp/bindings/lib/multiplex_router.cc:536:25
    #6 0x45cf394 in ReadSingleMessage
mojo/public/cpp/bindings/lib/connector.cc:247:51
    #7 0x45cfd64 in ReadAllAvailableMessages
mojo/public/cpp/bindings/lib/connector.cc:272:10
    #8 0x45cfd64 in OnHandleReadyInternal
mojo/public/cpp/bindings/lib/connector.cc:205:0
    #9 0x45cfd64 in OnWatcherHandleReady
mojo/public/cpp/bindings/lib/connector.cc:183:0
    #10 0x46069ff in Run base/callback.h:64:12
    #11 0x46069ff in OnHandleReady mojo/public/cpp/system/watcher.cc:122:0
    #12 0x416c973 in Run base/callback.h:47:12
    #13 0x416c973 in RunTask base/debug/task_annotator.cc:52:0
    #14 0x71db744 in ProcessTaskFromWorkQueue
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:378:19
    #15 0x71d77c9 in DoWork
third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:253:13
    #16 0x416c973 in Run base/callback.h:47:12
    #17 0x416c973 in RunTask base/debug/task_annotator.cc:52:0
    #18 0x403b960 in RunTask base/message_loop/message_loop.cc:413:19
    #19 0x403c255 in DeferOrRunPendingTask
base/message_loop/message_loop.cc:422:5
    #20 0x403d0de in DoWork base/message_loop/message_loop.cc:515:13
    #21 0x4045600 in Run base/message_loop/message_pump_default.cc:35:31
    #22 0x403b0d0 in RunHandler base/message_loop/message_loop.cc:378:10
    #23 0x40907e5 in Run base/run_loop.cc:35:10
    #24 0x6a4eabf in RendererMain content/renderer/renderer_main.cc:198:23
    #25 0x2eea179 in RunZygote content/app/content_main_runner.cc:336:14
    #26 0x2eecf19 in Run content/app/content_main_runner.cc:776:12
    #27 0x2ed8f5a in ContentMain content/app/content_main.cc:20:28
    #28 0x51831d in main content/shell/app/shell_main.cc:48:10
    #29 0x7f1b049c576c in __libc_start_main
/build/eglibc-rrybNj/eglibc-2.15/csu/libc-start.c:226:0

[horo, 2016-11-04 10:01:37]

> MojoAsyncResourceHandler sometimes calls  OnStartLoadingResponseBody
> before calling OnReceiveResponse.

Could you please add comments about it in url_loader.mojom?