Delegate WebSocket message handlers to the loading task runner

Delegate WebSocket message handlers to the loading task runner

IPC messages including WebSocket messages are dispatched on the default
task runner. We found WebSocket message handlers might call V8
functions. We plan to implement purge + suspend background tabs and need
to suspend all V8 function calls when purge + suspend happens, but the
default task runner can't be suspended since it'd be dangerous. Thus,
this CL delegates WebSocket message handlers from the default task
runner to the loading task runner.

BUG=607077
TEST=n/a

Committed: https://crrev.com/384b3b11a1afb71511dc15bc7324f69226ffe801
Cr-Commit-Position: refs/heads/master@{#397074}

[hajimehoshi, 2016-05-23 08:21:37]

hajimehoshi@chromium.org changed reviewers:
+ kinuko@chromium.org, skyostil@chromium.org

[hajimehoshi, 2016-05-23 08:21:38]

PTAL

[hajimehoshi, 2016-05-23 08:22:03]

Description was changed from

==========
Delegate websocket message handler to the loading task runner

IPC messages including WebSocket messages are dispatched on the default
task runner. We found WebSocket message handlers might call V8
functions. We plan to implement purge + suspend background tabs and need
to suspend all V8 function calls when purge + suspend happens, but the
default task runner can't be suspended since it'd be dangerous. Thus,
this CL delegates WebSocket message handlers from the default task
runner to the loading task runner.

BUG=607077
TEST=n/a
==========

to

==========
Delegate WebSocket message handlers to the loading task runner

IPC messages including WebSocket messages are dispatched on the default
task runner. We found WebSocket message handlers might call V8
functions. We plan to implement purge + suspend background tabs and need
to suspend all V8 function calls when purge + suspend happens, but the
default task runner can't be suspended since it'd be dangerous. Thus,
this CL delegates WebSocket message handlers from the default task
runner to the loading task runner.

BUG=607077
TEST=n/a
==========

[Sami, 2016-05-23 09:56:07]

Added a suggestion. Let me know what you think.

https://codereview.chromium.org/2000113002/diff/1/content/child/websocket_dis...
File content/child/websocket_dispatcher.cc (right):

https://codereview.chromium.org/2000113002/diff/1/content/child/websocket_dis...
content/child/websocket_dispatcher.cc:54:
loading_task_runner_->PostTask(FROM_HERE,
I guess what will end up happening here is that OnMessageReceived() will first
get called on the main thread and schedule this PostTask, which will also happen
on the main thread. Depending on how busy the main thread is this could add
significant delays to WebSocket messages.

A better alternative would be to turn WebSocketDispatcher into a IPC message
filter and have it PostTask to the loading task queue from the IO thread. I'm
not sure if we need to do that refactoring now, but I would suggest giving it
some thought (and adding a TODO at least).

[hajimehoshi, 2016-05-23 12:04:28]

On 2016/05/23 09:56:07, Sami wrote:
> Added a suggestion. Let me know what you think.
> 
>
https://codereview.chromium.org/2000113002/diff/1/content/child/websocket_dis...
> File content/child/websocket_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/2000113002/diff/1/content/child/websocket_dis...
> content/child/websocket_dispatcher.cc:54:
> loading_task_runner_->PostTask(FROM_HERE,
> I guess what will end up happening here is that OnMessageReceived() will first
> get called on the main thread and schedule this PostTask, which will also
happen
> on the main thread. Depending on how busy the main thread is this could add
> significant delays to WebSocket messages.
> 
> A better alternative would be to turn WebSocketDispatcher into a IPC message
> filter and have it PostTask to the loading task queue from the IO thread. I'm
> not sure if we need to do that refactoring now, but I would suggest giving it
> some thought (and adding a TODO at least).

Thank you for your suggestion. +1 to refactor to change WebSocketDispatcher to
be an IPC message filter. I think this refactoring would not be complicated.

[kinuko, 2016-05-23 12:27:22]

On 2016/05/23 12:04:28, hajimehoshi wrote:
> On 2016/05/23 09:56:07, Sami wrote:
> > Added a suggestion. Let me know what you think.
> > 
> >
>
https://codereview.chromium.org/2000113002/diff/1/content/child/websocket_dis...
> > File content/child/websocket_dispatcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/2000113002/diff/1/content/child/websocket_dis...
> > content/child/websocket_dispatcher.cc:54:
> > loading_task_runner_->PostTask(FROM_HERE,
> > I guess what will end up happening here is that OnMessageReceived() will
first
> > get called on the main thread and schedule this PostTask, which will also
> happen
> > on the main thread. Depending on how busy the main thread is this could add
> > significant delays to WebSocket messages.
> > 
> > A better alternative would be to turn WebSocketDispatcher into a IPC message
> > filter and have it PostTask to the loading task queue from the IO thread.
I'm
> > not sure if we need to do that refactoring now, but I would suggest giving
it
> > some thought (and adding a TODO at least).
> 
> Thank you for your suggestion. +1 to refactor to change WebSocketDispatcher to
> be an IPC message filter. I think this refactoring would not be complicated.

Or we could have a thin WebSocketMessageFilter and let it post tasks on the
loading task runner to call WebSocketDispatcher::OnMessageReceived on it, so
that we can keep most of the WebSocketDispatcher code as is.

[hajimehoshi, 2016-05-25 08:45:09]

PTAL

[kinuko, 2016-05-25 09:30:53]

(+cc yhirano as websocket owner)

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
File content/child/websocket_message_filter.cc (right):

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
content/child/websocket_message_filter.cc:25:
base::Unretained(websocket_dispatcher_),
It's not obvious who guarantees websocket_dispatcher_ is still alive when the
posted task runs?  If it's really safe can you add a comment here and at the raw
ptr in .h?  Or if it's not the case I think we should post task to |this| or to
static function first and make sure if websocket_dispatcher_ is alive via
weakptr etc

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
File content/child/websocket_message_filter.h (right):

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
content/child/websocket_message_filter.h:30:
WebSocketMessageFilter(WebSocketDispatcher* websocket_dispatcher);
nit: explicit

[yhirano, 2016-05-25 10:39:51]

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

[yhirano, 2016-05-25 10:39:52]

As talked offline, I'm concerned about performance. Sami, is the loading task
queue high prioritized?

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
File content/child/websocket_message_filter.cc (right):

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
content/child/websocket_message_filter.cc:20: bool
WebSocketMessageFilter::OnMessageReceived(const IPC::Message& message) {
Does this function run on the IO thread? If so, can you add an assertion?

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
File content/child/websocket_message_filter.h (right):

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
content/child/websocket_message_filter.h:24: // queues. However, the default
task queue can't be suspended because it'd be
Is this comment needed?

[Sami, 2016-05-25 11:11:20]

On 2016/05/25 10:39:52, yhirano wrote:
> As talked offline, I'm concerned about performance. Sami, is the loading task
> queue high prioritized?

Currently the loading task queue is always kept at a normal priority[1] since we
don't have a load time policy yet. This means that this change will not have a
performance impact for websocket messages.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/components/schedul...

[yhirano, 2016-05-25 11:34:52]

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
File content/child/websocket_message_filter.cc (right):

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
content/child/websocket_message_filter.cc:20: bool
WebSocketMessageFilter::OnMessageReceived(const IPC::Message& message) {
On 2016/05/25 10:39:52, yhirano wrote:
> Does this function run on the IO thread? If so, can you add an assertion?

Hmm, it seems there is no easy way to check that. I retract this comment.

[yhirano, 2016-05-25 11:36:04]

On 2016/05/25 11:11:20, Sami wrote:
> On 2016/05/25 10:39:52, yhirano wrote:
> > As talked offline, I'm concerned about performance. Sami, is the loading
task
> > queue high prioritized?
> 
> Currently the loading task queue is always kept at a normal priority[1] since
we
> don't have a load time policy yet. This means that this change will not have a
> performance impact for websocket messages.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/schedul...

Thank you!

[hajimehoshi, 2016-05-25 12:05:02]

Thank you! PTAL

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
File content/child/websocket_message_filter.cc (right):

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
content/child/websocket_message_filter.cc:25:
base::Unretained(websocket_dispatcher_),
On 2016/05/25 09:30:53, kinuko wrote:
> It's not obvious who guarantees websocket_dispatcher_ is still alive when the
> posted task runs?  If it's really safe can you add a comment here and at the
raw
> ptr in .h?  Or if it's not the case I think we should post task to |this| or
to
> static function first and make sure if websocket_dispatcher_ is alive via
> weakptr etc

IIUC websocket_dispatcher_ is owned by a RenderThreadImpl, which lives while the
renderer process lives.  Done.

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
File content/child/websocket_message_filter.h (right):

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
content/child/websocket_message_filter.h:24: // queues. However, the default
task queue can't be suspended because it'd be
On 2016/05/25 10:39:52, yhirano wrote:
> Is this comment needed?

As we discussed offline, this explanation is in the description in this CL and
it's enough. Done.

https://codereview.chromium.org/2000113002/diff/40001/content/child/websocket...
content/child/websocket_message_filter.h:30:
WebSocketMessageFilter(WebSocketDispatcher* websocket_dispatcher);
On 2016/05/25 09:30:53, kinuko wrote:
> nit: explicit

Done.

[yhirano, 2016-05-26 04:39:16]

https://codereview.chromium.org/2000113002/diff/60001/content/child/websocket...
File content/child/websocket_dispatcher.h (right):

https://codereview.chromium.org/2000113002/diff/60001/content/child/websocket...
content/child/websocket_dispatcher.h:37: bool CanHandleMessage(const
IPC::Message& msg) const;
Can you make this function static because this function can be accessed from any
threads?

[hajimehoshi, 2016-05-26 07:30:44]

Thank you!

https://codereview.chromium.org/2000113002/diff/60001/content/child/websocket...
File content/child/websocket_dispatcher.h (right):

https://codereview.chromium.org/2000113002/diff/60001/content/child/websocket...
content/child/websocket_dispatcher.h:37: bool CanHandleMessage(const
IPC::Message& msg) const;
On 2016/05/26 04:39:15, yhirano wrote:
> Can you make this function static because this function can be accessed from
any
> threads?

Done.

[yhirano, 2016-05-26 07:37:46]

lgtm

[kinuko, 2016-05-26 08:15:35]

looking good in general, though..

https://codereview.chromium.org/2000113002/diff/80001/content/child/websocket...
File content/child/websocket_message_filter.cc (right):

https://codereview.chromium.org/2000113002/diff/80001/content/child/websocket...
content/child/websocket_message_filter.cc:27:
base::Unretained(websocket_dispatcher_),
I might be just too nervouse, but I'm not fully happy with this as this could
get broken if any changes are made in renderer thread lifecycle and the
description 'when the renderer process lives' feels a bit unclear (while it
seems it's true that RenderThreadImpl isn't freed in production code), and where
websocket_dispatcher_ lives is actually on ChildThreadImpl...

Could we still make this a bit nicer?  E.g. having a glue method on this to post
task to using weak-ptr of this class, have Shutdown() method which invalidates
all weak-ptrs and let ChildThreadImpl::Shutdown() call it?

[hajimehoshi, 2016-05-26 09:49:02]

Thank you! hirano-san, can you take a look again since the design has been a
little changed to use base::WeakPtr.

https://codereview.chromium.org/2000113002/diff/80001/content/child/websocket...
File content/child/websocket_message_filter.cc (right):

https://codereview.chromium.org/2000113002/diff/80001/content/child/websocket...
content/child/websocket_message_filter.cc:27:
base::Unretained(websocket_dispatcher_),
On 2016/05/26 08:15:35, kinuko wrote:
> I might be just too nervouse, but I'm not fully happy with this as this could
> get broken if any changes are made in renderer thread lifecycle and the
> description 'when the renderer process lives' feels a bit unclear (while it
> seems it's true that RenderThreadImpl isn't freed in production code), and
where
> websocket_dispatcher_ lives is actually on ChildThreadImpl...
> 
> Could we still make this a bit nicer?  E.g. having a glue method on this to
post
> task to using weak-ptr of this class, have Shutdown() method which invalidates
> all weak-ptrs and let ChildThreadImpl::Shutdown() call it?

Done.

[kinuko, 2016-05-26 10:09:54]

lgtm

https://codereview.chromium.org/2000113002/diff/100001/content/child/websocke...
File content/child/websocket_message_filter.h (right):

https://codereview.chromium.org/2000113002/diff/100001/content/child/websocke...
content/child/websocket_message_filter.h:41: base::WeakPtr<WebSocketDispatcher>
websocket_dispatcher_;
nit: add a comment to note this must be de-ref'ed only on the main thread

[hajimehoshi, 2016-05-26 10:18:42]

Thank you!

https://codereview.chromium.org/2000113002/diff/100001/content/child/websocke...
File content/child/websocket_message_filter.h (right):

https://codereview.chromium.org/2000113002/diff/100001/content/child/websocke...
content/child/websocket_message_filter.h:41: base::WeakPtr<WebSocketDispatcher>
websocket_dispatcher_;
On 2016/05/26 10:09:54, kinuko wrote:
> nit: add a comment to note this must be de-ref'ed only on the main thread

Done.

[yhirano, 2016-05-27 01:51:07]

lgtm

[hajimehoshi, 2016-05-30 06:10:00]

ping Sami

[hajimehoshi, 2016-06-01 05:17:31]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2016-06-01 05:17:31]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2000113002/#ps120001
(title: "Add a comment")

[commit-bot: I haz the power, 2016-06-01 05:17:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000113002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000113002/120001

[commit-bot: I haz the power, 2016-06-01 06:22:02]

Description was changed from

==========
Delegate WebSocket message handlers to the loading task runner

IPC messages including WebSocket messages are dispatched on the default
task runner. We found WebSocket message handlers might call V8
functions. We plan to implement purge + suspend background tabs and need
to suspend all V8 function calls when purge + suspend happens, but the
default task runner can't be suspended since it'd be dangerous. Thus,
this CL delegates WebSocket message handlers from the default task
runner to the loading task runner.

BUG=607077
TEST=n/a
==========

to

==========
Delegate WebSocket message handlers to the loading task runner

IPC messages including WebSocket messages are dispatched on the default
task runner. We found WebSocket message handlers might call V8
functions. We plan to implement purge + suspend background tabs and need
to suspend all V8 function calls when purge + suspend happens, but the
default task runner can't be suspended since it'd be dangerous. Thus,
this CL delegates WebSocket message handlers from the default task
runner to the loading task runner.

BUG=607077
TEST=n/a
==========

[commit-bot: I haz the power, 2016-06-01 06:22:04]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-01 06:23:35]

Description was changed from

==========
Delegate WebSocket message handlers to the loading task runner

IPC messages including WebSocket messages are dispatched on the default
task runner. We found WebSocket message handlers might call V8
functions. We plan to implement purge + suspend background tabs and need
to suspend all V8 function calls when purge + suspend happens, but the
default task runner can't be suspended since it'd be dangerous. Thus,
this CL delegates WebSocket message handlers from the default task
runner to the loading task runner.

BUG=607077
TEST=n/a
==========

to

==========
Delegate WebSocket message handlers to the loading task runner

IPC messages including WebSocket messages are dispatched on the default
task runner. We found WebSocket message handlers might call V8
functions. We plan to implement purge + suspend background tabs and need
to suspend all V8 function calls when purge + suspend happens, but the
default task runner can't be suspended since it'd be dangerous. Thus,
this CL delegates WebSocket message handlers from the default task
runner to the loading task runner.

BUG=607077
TEST=n/a

Committed: https://crrev.com/384b3b11a1afb71511dc15bc7324f69226ffe801
Cr-Commit-Position: refs/heads/master@{#397074}
==========

[commit-bot: I haz the power, 2016-06-01 06:23:37]

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