Cancel loading when body stream reader is detached.

Cancel loading when body stream reader is detached.

With this CL, loading with useStreamOnResponse will stop when nobody
monitors the reading end (i.e. when the WebDataConsumerHandle and its
reader are detached).

BUG=480746
Committed: https://crrev.com/a9ffb2129748798f233257b29d51d5df8f607f25
Cr-Commit-Position: refs/heads/master@{#335654}

[yhirano, 2015-06-15 09:20:39]

Patchset #2 (id:20001) has been deleted

[yhirano, 2015-06-15 09:34:07]

Patchset #2 (id:40001) has been deleted

[yhirano, 2015-06-15 09:36:24]

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

[yhirano, 2015-06-15 09:36:24]

This CL depends on https://codereview.chromium.org/1181573003/.

We talked about having OnClearHolder, but I found some difficulties
(as you may already know) and changed my mind.

[hiroshige, 2015-06-22 09:51:19]

Could you rebase after https://codereview.chromium.org/1181573003/ ?

[yhirano, 2015-06-22 10:18:42]

On 2015/06/22 09:51:19, hiroshige wrote:
> Could you rebase after https://codereview.chromium.org/1181573003/ ?

Done!

[hiroshige, 2015-06-22 18:23:16]

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:67: // We post a task even
in the writer thread in order to keep reentrancy.
how about adding "because didGetReadable() can destruct the reader, which causes
ClearIfNecessary(), and calling |on_clear_| can cause didGetReadable() call" or
so (this explanation is somehow specific to CancelBodyStreaming() though).

nit optional: s/keep/avoid/? (not sure though)

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:185: // Note this doesn't
work in the destructor if |on_clear_| is not null. We
This comment might be better to be placed just before ResetOnClear() definition.

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:211: // because we need to
set |on_clear_| only on the writer thread and hence
optional: how about "... because |on_clear_| is reset on the writer thread
asynchronously and hence ..."?

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:213: bool
is_on_clear_valid_;
Please add a comment that |on_clear_| and |is_on_clear_valid_| is protected by
|lock_|. (except for in the destructor?)

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.h (right):

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:68: // the associated handle
and the reader are destructed.
Better name (here and related method/member names) is preferred for |on_clear_|
because |on_clear_| looks like a callback for Clear() but actually a callback
for Handle/Reader-initiated Clear().

How about:
- modifying this comment to "... when the body data stream is detached from the
reader side, i.e. when both the associated handle and the reader are
destructed."
- s/on_clear_/on_detach_from_reader_side/g (also do similar for related names)

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:70: // on the writer is
called.
How about "will be reset in the internal context when the body data stream is
detached from the writer side, i.e. |Close| or |Error| on the writer is called."
for symmetry?

https://codereview.chromium.org/1186053004/diff/150001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1186053004/diff/150001/content/child/web_url_...
content/child/web_url_loader_impl.cc:766: void
WebURLLoaderImpl::Context::CancelBodyStreaming() {
L768-784 cancels downstream (the same as OnCompletedRequest() with error) and
Cancel() cancels upstream, right? Adding a comment for it might be helpful.

optional: merging L768-784 with OnCompletedRequest() (or calling
OnCompletedRequest() here) might be helpful for avoiding duplicated code.

[yhirano, 2015-06-23 05:28:38]

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:67: // We post a task even
in the writer thread in order to keep reentrancy.
On 2015/06/22 18:23:16, hiroshige wrote:
> how about adding "because didGetReadable() can destruct the reader, which
causes
> ClearIfNecessary(), and calling |on_clear_| can cause didGetReadable() call"
or
> so (this explanation is somehow specific to CancelBodyStreaming() though).
> 
> nit optional: s/keep/avoid/? (not sure though)

Done.

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:185: // Note this doesn't
work in the destructor if |on_clear_| is not null. We
On 2015/06/22 18:23:16, hiroshige wrote:
> This comment might be better to be placed just before ResetOnClear()
definition.

Done.

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:211: // because we need to
set |on_clear_| only on the writer thread and hence
On 2015/06/22 18:23:16, hiroshige wrote:
> optional: how about "... because |on_clear_| is reset on the writer thread
> asynchronously and hence ..."?

I think it misleading the callback will be reset synchronously when reset on the
writer thread.

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:213: bool
is_on_clear_valid_;
On 2015/06/22 18:23:16, hiroshige wrote:
> Please add a comment that |on_clear_| and |is_on_clear_valid_| is protected by
> |lock_|. (except for in the destructor?)

Other variables are also protected by the lock, so I don't see a reason to say
that only for these variables.

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.h (right):

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:68: // the associated handle
and the reader are destructed.
On 2015/06/22 18:23:16, hiroshige wrote:
> Better name (here and related method/member names) is preferred for
|on_clear_|
> because |on_clear_| looks like a callback for Clear() but actually a callback
> for Handle/Reader-initiated Clear().
> 
> How about:
> - modifying this comment to "... when the body data stream is detached from
the
> reader side, i.e. when both the associated handle and the reader are
> destructed."
> - s/on_clear_/on_detach_from_reader_side/g (also do similar for related names)

Done.

https://codereview.chromium.org/1186053004/diff/150001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.h:70: // on the writer is
called.
On 2015/06/22 18:23:16, hiroshige wrote:
> How about "will be reset in the internal context when the body data stream is
> detached from the writer side, i.e. |Close| or |Error| on the writer is
called."
> for symmetry?

Done.

https://codereview.chromium.org/1186053004/diff/150001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1186053004/diff/150001/content/child/web_url_...
content/child/web_url_loader_impl.cc:766: void
WebURLLoaderImpl::Context::CancelBodyStreaming() {
On 2015/06/22 18:23:16, hiroshige wrote:
> L768-784 cancels downstream (the same as OnCompletedRequest() with error) and
> Cancel() cancels upstream, right? Adding a comment for it might be helpful.
> 

Done.

> optional: merging L768-784 with OnCompletedRequest() (or calling
> OnCompletedRequest() here) might be helpful for avoiding duplicated code.

I don't know what I should provide as parameters (was_ignored_by_handler,
stale_copy_in_cache and security_info), so I would keep the current, more direct
way.

[hiroshige, 2015-06-23 05:51:21]

lgtm.

[yhirano, 2015-06-23 06:06:02]

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

[yhirano, 2015-06-23 06:06:03]

+kinuko for OWNER review.

[kinuko, 2015-06-23 07:56:10]

lgtm

https://codereview.chromium.org/1186053004/diff/170001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1186053004/diff/170001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:56:
writer_task_runner_(base::MessageLoop::current()->task_runner()),
please use base::ThreadTaskRunnerHandle::Get() instead

[yhirano, 2015-06-23 08:17:55]

https://codereview.chromium.org/1186053004/diff/170001/content/child/shared_m...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1186053004/diff/170001/content/child/shared_m...
content/child/shared_memory_data_consumer_handle.cc:56:
writer_task_runner_(base::MessageLoop::current()->task_runner()),
On 2015/06/23 07:56:10, kinuko wrote:
> please use base::ThreadTaskRunnerHandle::Get() instead

Done, and replaced similar ones.

[yhirano, 2015-06-23 08:18:18]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2015-06-23 08:18:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
hiroshige@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1186053004/#ps190001
(title: " ")

[commit-bot: I haz the power, 2015-06-23 08:18:57]

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

[commit-bot: I haz the power, 2015-06-23 09:41:07]

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

[commit-bot: I haz the power, 2015-06-23 09:41:08]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[hiroshige, 2015-06-23 09:44:45]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2015-06-23 09:45:02]

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

[commit-bot: I haz the power, 2015-06-23 10:13:36]

Committed patchset #9 (id:190001)

[commit-bot: I haz the power, 2015-06-23 10:14:54]

Patchset 9 (id:??) landed as
https://crrev.com/a9ffb2129748798f233257b29d51d5df8f607f25
Cr-Commit-Position: refs/heads/master@{#335654}