When a plugin calls NPN_SetException, pass the exception along to every rende...

If an NP_* function is called on an out of process plugin, save enough info to send an NPN_SetException back to the correct renderer if necessary.  BUG=26764
TEST=none 

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

[Nate Chapin, 2009-11-16 16:45:44]

I debated rewriting NPObjectMsg_SetException to also take the NPObject* as a
parameter, but I wasn't sure whether that was worth the trouble.

[darin (slow to review), 2009-11-16 17:05:20]

Is it possible to write a test for this?  Maybe by extending NPAPITester?

[Nate Chapin, 2009-11-16 17:07:32]

I'll look into it, thanks.
~Nate

On Mon, Nov 16, 2009 at 9:05 AM, <darin@chromium.org> wrote:

> Is it possible to write a test for this?  Maybe by extending NPAPITester?
>
>
> http://codereview.chromium.org/375005
>

[Nate Chapin, 2009-11-16 17:33:40]

I should probably point out that I have thus far never touched any tests
other than layout tests, so this may be a slow process for me...

On Mon, Nov 16, 2009 at 9:07 AM, Nate Chapin <japhet@chromium.org> wrote:

> I'll look into it, thanks.
> ~Nate
>
>
> On Mon, Nov 16, 2009 at 9:05 AM, <darin@chromium.org> wrote:
>
>> Is it possible to write a test for this?  Maybe by extending NPAPITester?
>>
>>
>> http://codereview.chromium.org/375005
>>
>
>

[Nate Chapin, 2009-11-16 19:06:42]

I've added the ui test, but the more I think about it, the more I think the IPC
should include the NPObject* (since I'm sort of fuding it by passing a NULL
NPObject* and there's no reason not to pass the pointer we have)

[Nate Chapin, 2009-11-16 20:26:01]

On 2009/11/16 19:06:42, Nate Chapin wrote:
> I've added the ui test, but the more I think about it, the more I think the
IPC
> should include the NPObject* (since I'm sort of fuding it by passing a NULL
> NPObject* and there's no reason not to pass the pointer we have)

After some experimentation, it looks like just adding the NPObject* to the IPC
wouldn't be enough, since it sort of points to an object in the memory space of
the plugin process, not the renderer process.

This test is currently crashing on the buildbots because it depends on
https://bugs.webkit.org/show_bug.cgi?id=31561

[jam, 2009-11-16 21:18:44]

http://codereview.chromium.org/375005/diff/6003/10009
File chrome/plugin/npobject_stub.cc (right):

http://codereview.chromium.org/375005/diff/6003/10009#newcode76
Line 76: current_renderer_channel_ = channel_;
Why not do this in PluginChannelBase?  It's OnMessageReceived already keeps an
extra reference to itself to ensure it doesn't get deleted while it's being
used.  So instead of just having them be in local scope, if you stick them in a
stack you can expose it via a getter and use it in NPN_SetException

http://codereview.chromium.org/375005/diff/6003/10009#newcode96
Line 96: current_renderer_channel_ = NULL;
you'll really want to restore the previous current_renderer_channel_ value,
since there could be recursive calls.

[Nate Chapin, 2009-11-16 21:48:32]

On Mon, Nov 16, 2009 at 1:18 PM, <jam@chromium.org> wrote:

>
> http://codereview.chromium.org/375005/diff/6003/10009
> File chrome/plugin/npobject_stub.cc (right):
>
> http://codereview.chromium.org/375005/diff/6003/10009#newcode76
> Line 76: current_renderer_channel_ = channel_;
> Why not do this in PluginChannelBase?  It's OnMessageReceived already
> keeps an extra reference to itself to ensure it doesn't get deleted
> while it's being used.  So instead of just having them be in local
> scope, if you stick them in a stack you can expose it via a getter and
> use it in NPN_SetException
>
> http://codereview.chromium.org/375005/diff/6003/10009#newcode96
> Line 96: current_renderer_channel_ = NULL;
> you'll really want to restore the previous current_renderer_channel_
> value, since there could be recursive calls.
>
>
> http://codereview.chromium.org/375005
>

That seems reasonable (I hadn't paid enough attention to the behavior of
PlugginChannelBase::OnMessageReceived()).  Should I keep doing what I'm
doing for the route id?  That doesn't exist at the level of
PluginChannelBase afaict.

[jam, 2009-11-16 21:53:21]

On 2009/11/16 21:48:32, Nate Chapin wrote:
> That seems reasonable (I hadn't paid enough attention to the behavior of
> PlugginChannelBase::OnMessageReceived()).  Should I keep doing what I'm
> doing for the route id?  That doesn't exist at the level of
> PluginChannelBase afaict.
> 

You don't need a routing id, since at the other end you just call a static
function anyways (WebBindings::setException).  So just send a control message
and receive it in PluginChannelHost::OnMessageReceived.

[Nate Chapin, 2009-11-16 22:42:51]

On 2009/11/16 21:53:21, John Abd-El-Malek wrote:
> On 2009/11/16 21:48:32, Nate Chapin wrote:
> > That seems reasonable (I hadn't paid enough attention to the behavior of
> > PlugginChannelBase::OnMessageReceived()).  Should I keep doing what I'm
> > doing for the route id?  That doesn't exist at the level of
> > PluginChannelBase afaict.
> > 
> 
> You don't need a routing id, since at the other end you just call a static
> function anyways (WebBindings::setException).  So just send a control message
> and receive it in PluginChannelHost::OnMessageReceived.

Done.

Also, http://trac.webkit.org/changeset/51054 is landed upstream, so this will
stop crashing as soon as the WebKit DEPS roll that far.

[jam, 2009-11-17 00:04:13]

http://codereview.chromium.org/375005/diff/11008/8013
File chrome/plugin/npobject_stub.cc (right):

http://codereview.chromium.org/375005/diff/11008/8013#newcode375
Line 375: void NPObjectStub::NPN_SetException(NPObject* obj, const NPUTF8*
message) {
Since it doesn't use NPObjectStub, this code should just stay in
npobject_utils's NPN_SetExceptionPatch.

http://codereview.chromium.org/375005/diff/11008/8013#newcode382
Line 382: }
nit: no need for brace brackets

http://codereview.chromium.org/375005/diff/11008/8018
File chrome/plugin/plugin_channel_base.cc (right):

http://codereview.chromium.org/375005/diff/11008/8018#newcode22
Line 22: static std::stack<PluginChannelBase*> g_plugin_channel_stack_;
nit: chrome style is to avoid global objects with constructors/destructors.  You
can use LazyInstance to get around this.

http://codereview.chromium.org/375005/diff/11008/8019
File chrome/plugin/plugin_channel_base.h (right):

http://codereview.chromium.org/375005/diff/11008/8019#newcode52
Line 52: static PluginChannelBase* GetCurrentChannel();
comment?

[Nate Chapin, 2009-11-17 00:37:20]

I think I got them all....sorry for my ignorance of the Chromium style guide.

http://codereview.chromium.org/375005/diff/11008/8013
File chrome/plugin/npobject_stub.cc (right):

http://codereview.chromium.org/375005/diff/11008/8013#newcode375
Line 375: void NPObjectStub::NPN_SetException(NPObject* obj, const NPUTF8*
message) {
On 2009/11/17 00:04:13, John Abd-El-Malek wrote:
> Since it doesn't use NPObjectStub, this code should just stay in
> npobject_utils's NPN_SetExceptionPatch.
> 

Done.

http://codereview.chromium.org/375005/diff/11008/8013#newcode382
Line 382: }
On 2009/11/17 00:04:13, John Abd-El-Malek wrote:
> nit: no need for brace brackets

Done.

http://codereview.chromium.org/375005/diff/11008/8018
File chrome/plugin/plugin_channel_base.cc (right):

http://codereview.chromium.org/375005/diff/11008/8018#newcode22
Line 22: static std::stack<PluginChannelBase*> g_plugin_channel_stack_;
On 2009/11/17 00:04:13, John Abd-El-Malek wrote:
> nit: chrome style is to avoid global objects with constructors/destructors. 
You
> can use LazyInstance to get around this.

Done.

http://codereview.chromium.org/375005/diff/11008/8019
File chrome/plugin/plugin_channel_base.h (right):

http://codereview.chromium.org/375005/diff/11008/8019#newcode52
Line 52: static PluginChannelBase* GetCurrentChannel();
On 2009/11/17 00:04:13, John Abd-El-Malek wrote:
> comment?

Done.

[jam, 2009-11-17 00:49:20]

sorry for dragging this on!

http://codereview.chromium.org/375005/diff/6006/6014
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/375005/diff/6006/6014#newcode413
Line 413: IPC_SYNC_MESSAGE_CONTROL1_0(NPObjectMsg_SetException,
nit: now that this isn't going between NPObject's, it should be a PluginHost
message and moved to that section

http://codereview.chromium.org/375005/diff/6006/6015
File chrome/plugin/plugin_channel_base.cc (right):

http://codereview.chromium.org/375005/diff/6006/6015#newcode129
Line 129: lazy_plugin_channel_stack_.Pointer()->push(this);
why don't you just make the stack hold scoped_refptrs, and then you don't need
line 127?

http://codereview.chromium.org/375005/diff/6006/6018
File chrome/renderer/plugin_channel_host.h (right):

http://codereview.chromium.org/375005/diff/6006/6018#newcode27
Line 27: void OnControlMessageReceived(const IPC::Message& message);
nit: these can just be private

[Nate Chapin, 2009-11-17 17:20:59]

I do not object to nitpicks and dragging on when the nits are legitimate and the
reviews are prompt :-)

http://codereview.chromium.org/375005/diff/6006/6014
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/375005/diff/6006/6014#newcode413
Line 413: IPC_SYNC_MESSAGE_CONTROL1_0(NPObjectMsg_SetException,
On 2009/11/17 00:49:20, John Abd-El-Malek wrote:
> nit: now that this isn't going between NPObject's, it should be a PluginHost
> message and moved to that section

Done, though do these really belong in PluginHost, or should they be in a new
PluginChannelHost block?

http://codereview.chromium.org/375005/diff/6006/6015
File chrome/plugin/plugin_channel_base.cc (right):

http://codereview.chromium.org/375005/diff/6006/6015#newcode129
Line 129: lazy_plugin_channel_stack_.Pointer()->push(this);
On 2009/11/17 00:49:20, John Abd-El-Malek wrote:
> why don't you just make the stack hold scoped_refptrs, and then you don't need
> line 127?

Done.

http://codereview.chromium.org/375005/diff/6006/6018
File chrome/renderer/plugin_channel_host.h (right):

http://codereview.chromium.org/375005/diff/6006/6018#newcode27
Line 27: void OnControlMessageReceived(const IPC::Message& message);
On 2009/11/17 00:49:20, John Abd-El-Malek wrote:
> nit: these can just be private

Done.

[jam, 2009-11-18 01:30:08]

lgtm

http://codereview.chromium.org/375005/diff/6006/6014
File chrome/common/plugin_messages_internal.h (right):

http://codereview.chromium.org/375005/diff/6006/6014#newcode413
Line 413: IPC_SYNC_MESSAGE_CONTROL1_0(NPObjectMsg_SetException,
On 2009/11/17 17:20:59, Nate Chapin wrote:
> On 2009/11/17 00:49:20, John Abd-El-Malek wrote:
> > nit: now that this isn't going between NPObject's, it should be a PluginHost
> > message and moved to that section
> 
> Done, though do these really belong in PluginHost, or should they be in a new
> PluginChannelHost block?
> 

messages from plugin to renderer process are in the PluginHost block.  The
message block names don't map exactly to PluginChannel/PluginChannelHost.