Split ExtensionMessageFilter up into a UI thread part and an IO thread part.

Split ExtensionMessageFilter up into a UI thread part and an IO thread part.

The UI thread part is shut down together with the keyed services it depends on, via a new KeyedServiceShutdownNotifier class.

BUG=395391
Committed: https://crrev.com/db86306f78fdc641868539eb34ad55890456de05
Cr-Commit-Position: refs/heads/master@{#315631}

[Bernhard Bauer, 2015-02-08 17:01:07]

bauerb@chromium.org changed reviewers:
+ kalman@chromium.org

[Bernhard Bauer, 2015-02-08 17:01:18]

Please review.

[Bernhard Bauer, 2015-02-10 18:28:24]

Benjamin, friendly ping? :)

[not at google - send to devlin, 2015-02-10 18:37:15]

lgtm. cleanup suggestions.

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/exte...
File extensions/browser/extension_message_filter.cc (right):

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/exte...
extensions/browser/extension_message_filter.cc:229: RenderProcessHost* process =
RenderProcessHost::FromID(render_process_id_);
I wonder if this can actually fail given it's called from an IPC message
handler. Eh, whatever.

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/exte...
extensions/browser/extension_message_filter.cc:249:
process_manager_->OnShouldSuspendAck(extension_id, sequence_id);
could you write these like

if (process_manager_)
  process_manager_->OnShouldSuspendAck(...);

though if you do the check ExtensionSystem thing in the message handler it
shouldn't be possible for there to be a ProcessManager but not an
ExtensionSystem.

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/io_t...
File extensions/browser/io_thread_extension_message_filter.h (right):

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/io_t...
extensions/browser/io_thread_extension_message_filter.h:31: int
render_process_id() { return render_process_id_; }
Where is this called from?

[not at google - send to devlin, 2015-02-10 18:37:41]

Forgot to save this comment.

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/exte...
File extensions/browser/extension_message_filter.cc (right):

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/exte...
extensions/browser/extension_message_filter.cc:163: if (!extension_system_)
Is it possible to do all of these extension system checks in the
OnMessageReceived function?

[Bernhard Bauer, 2015-02-10 18:58:14]

New patchsets have been uploaded after l-g-t-m from kalman@chromium.org

[Bernhard Bauer, 2015-02-10 19:02:34]

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/exte...
File extensions/browser/extension_message_filter.cc (right):

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/exte...
extensions/browser/extension_message_filter.cc:163: if (!extension_system_)
On 2015/02/10 18:37:41, kalman wrote:
> Is it possible to do all of these extension system checks in the
> OnMessageReceived function?

Done. It does mean that we don't check anymore whether the message is handled at
all, but that should be fine, because other messages shouldn't get forwarded
here.

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/exte...
extensions/browser/extension_message_filter.cc:229: RenderProcessHost* process =
RenderProcessHost::FromID(render_process_id_);
On 2015/02/10 18:37:15, kalman wrote:
> I wonder if this can actually fail given it's called from an IPC message
> handler. Eh, whatever.

Process shuts down between the time the message is received on the IO thread and
the time it's handled on the UI thread? There is nothing that would cancel it in
that case.

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/exte...
extensions/browser/extension_message_filter.cc:249:
process_manager_->OnShouldSuspendAck(extension_id, sequence_id);
On 2015/02/10 18:37:14, kalman wrote:
> could you write these like
> 
> if (process_manager_)
>   process_manager_->OnShouldSuspendAck(...);
> 
> though if you do the check ExtensionSystem thing in the message handler it
> shouldn't be possible for there to be a ProcessManager but not an
> ExtensionSystem.

Yeah... I removed the check completely, because if we end up here and 
|process_manager_| is in fact null, we'll crash anyway.

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/io_t...
File extensions/browser/io_thread_extension_message_filter.h (right):

https://codereview.chromium.org/901573003/diff/210001/extensions/browser/io_t...
extensions/browser/io_thread_extension_message_filter.h:31: int
render_process_id() { return render_process_id_; }
On 2015/02/10 18:37:15, kalman wrote:
> Where is this called from?

WebRequestInternalAddEventListenerFunction::RunSync(). This is also the reason
why we can't just hand out a WeakPtr<IPC::Sender>, but have to expose the full
class :-/

[Bernhard Bauer, 2015-02-10 19:05:34]

The CQ bit was checked by bauerb@chromium.org

[commit-bot: I haz the power, 2015-02-10 19:06:10]

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

[commit-bot: I haz the power, 2015-02-10 20:42:06]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2015-02-10 20:42:57]

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