Create a message filter for message port messages. This allows a nice cleanu...

Create a message filter for message port and worker messages.  This allows a nice cleanup of getting rid of the notifications when RenderMessageFilter and WorkerProcessHost go away.  SafeBrowsingResourceHandler doesn't need it anymore as well, since it now overrides OnRequestClosed() which does the same thing.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69862

[Andrew T Wilson (Slow), 2010-12-21 03:12:21]

Sorry this took so long - there were some routines rearranged in some files, and
I couldn't be certain whether there were changes to the routines or if they were
just moved so that slowed me down some.

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/resource_request_details.cc (right):

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/resource_request_details.cc:36: info->child_id(),
&origin_child_id_, &temp)) {
It seems cleaner to pass in a NULL ptr for unneeded out params - if you really
want to use a throwaway variable instead, maybe you could change the name to
something like "ignored_view_host_id" or something.

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
File chrome/browser/worker_host/message_port_service.cc (right):

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/message_port_service.cc:38: WorkerMessageFilter*
filter,
Remove extra space at front

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/message_port_service.cc:161: if
(entangled_port.filter) {
If entangled_port.filter is null here, then we would have crashed in the
previous for loop when we called entangled_port.filter->GetNextRoutingID(). So I
think we need to do something different here if we ever can get called with a
null filter (I'm not sure what that case is - is that the case when you've send
a port to another process, so the original port is now unentangled)?

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/message_port_service.cc:180: port.filter = NULL;
Coming back to this code after a several-month hiatus, I am now realizing that I
don't entirely recall what case this is covering. It would be useful to add a
comment to some of these routines explaining when they are called (called when a
port is being sent to another process, etc).

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
File chrome/browser/worker_host/worker_document_set.h (right):

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/worker_document_set.h:39: // id/sender.
comment is obsolete - change sender => filter.

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
File chrome/browser/worker_host/worker_message_filter.h (right):

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/worker_message_filter.h:51: 
nit: remove extra blank line?

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/worker_message_filter.h:52: int render_process_id_;
I'm not clear why we are keeping around the render_process_id here - see my
other comment in worker_process_host.cc about a worker outliving its parent.

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
File chrome/browser/worker_host/worker_process_host.cc (right):

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/worker_process_host.cc:179:
CreateMessageFilters(render_process_id);
I'm concerned that we're keeping the creator's render_process_id around, since
SharedWorkers can outlive their parents.

I may be misreading the code in WorkerService that calls Init(), though - I
think the render_process_id here is the id of the parent that created this
worker, but perhaps that's not correct?

[jam, 2010-12-21 07:41:51]

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/resource_request_details.cc (right):

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/resource_request_details.cc:36: info->child_id(),
&origin_child_id_, &temp)) {
On 2010/12/21 03:12:21, Andrew T Wilson wrote:
> It seems cleaner to pass in a NULL ptr for unneeded out params - if you really
> want to use a throwaway variable instead, maybe you could change the name to
> something like "ignored_view_host_id" or something.

I think it's personal taste :) I prefer not adding special code to functions for
this unless it helps a lot of callers.  In this case, it's just one.  Regarding
the naming, given that there's only one line left in the function, I think
temp/ignored/etc is equivalent.

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
File chrome/browser/worker_host/message_port_service.cc (right):

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/message_port_service.cc:38: WorkerMessageFilter*
filter,
On 2010/12/21 03:12:21, Andrew T Wilson wrote:
> Remove extra space at front

Done.

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/message_port_service.cc:161: if
(entangled_port.filter) {
On 2010/12/21 03:12:21, Andrew T Wilson wrote:
> If entangled_port.filter is null here, then we would have crashed in the
> previous for loop when we called entangled_port.filter->GetNextRoutingID(). So
I
> think we need to do something different here if we ever can get called with a
> null filter (I'm not sure what that case is - is that the case when you've
send
> a port to another process, so the original port is now unentangled)?

good point, made the "else" be an "else if" and added a comment wondering if it
can be null.  i don't know which situation this can happen, if any, so went the
safe route for now.

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/message_port_service.cc:180: port.filter = NULL;
On 2010/12/21 03:12:21, Andrew T Wilson wrote:
> Coming back to this code after a several-month hiatus, I am now realizing that
I
> don't entirely recall what case this is covering. It would be useful to add a
> comment to some of these routines explaining when they are called (called when
a
> port is being sent to another process, etc).

I'd love to add some good comments, but I haven't touched this code in over a
year so I'd prefer not to add extra comments in case they're off

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
File chrome/browser/worker_host/worker_document_set.h (right):

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/worker_document_set.h:39: // id/sender.
On 2010/12/21 03:12:21, Andrew T Wilson wrote:
> comment is obsolete - change sender => filter.

Done.

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
File chrome/browser/worker_host/worker_message_filter.h (right):

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/worker_message_filter.h:51: 
On 2010/12/21 03:12:21, Andrew T Wilson wrote:
> nit: remove extra blank line?

Done.

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/worker_message_filter.h:52: int render_process_id_;
On 2010/12/21 03:12:21, Andrew T Wilson wrote:
> I'm not clear why we are keeping around the render_process_id here - see my
> other comment in worker_process_host.cc about a worker outliving its parent.

I had to add this to match the old behavior where when a worker is created we
want the WorkerInstance to have the render_process_id, and also to initialize
the corresponding workerdocumentset with it.  There were a lot of comments and
TODOs in the code about how this isn't completely right for shared workers with
multiple parent processes, but I left that cleanup for another time.

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
File chrome/browser/worker_host/worker_process_host.cc (right):

http://codereview.chromium.org/6055002/diff/77001/chrome/browser/worker_host/...
chrome/browser/worker_host/worker_process_host.cc:179:
CreateMessageFilters(render_process_id);
On 2010/12/21 03:12:21, Andrew T Wilson wrote:
> I'm concerned that we're keeping the creator's render_process_id around, since
> SharedWorkers can outlive their parents.
> 
> I may be misreading the code in WorkerService that calls Init(), though - I
> think the render_process_id here is the id of the parent that created this
> worker, but perhaps that's not correct?

yep, it's the parent process.  My goal was to not change the functionality in
this change though, I think this is the same as before.

[Andrew T Wilson (Slow), 2010-12-21 19:28:22]

LGTM. One nit below, but I don't want to hold up this patch, so I'll tackle it
in a followon patch.

Thanks again for tackling this - the code is way cleaner now.

http://codereview.chromium.org/6055002/diff/53003/chrome/browser/worker_host/...
File chrome/browser/worker_host/message_port_service.cc (right):

http://codereview.chromium.org/6055002/diff/53003/chrome/browser/worker_host/...
chrome/browser/worker_host/message_port_service.cc:148: } else if
(entangled_port.filter) {  // TODO: can filter ever be NULL?
Rather than leave this as a TODO, can we make it:

} else {
  if (!entangled_port.filter) {
    NOTREACHED();
    return;
  }
  ...

It seems bad to just drop messages on the floor, and if we're asserting that
this can't happen, lets actually add some EC code to test this case.