Cleanup - remove ModuleLocalThreadAdapter.

Cleanup - remove ModuleLocalThreadAdapter.

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

[Tom Sepez, 2012-06-07 17:30:53]

Kept "thunk" intact -- unclear if this can change.

[yzshen1, 2012-06-07 17:38:34]

Please note that ppb_flash_impl also implement
Create/ClearTheadAdapterForInstance.

http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/plugins/ppapi/ppb_f...

http://codereview.chromium.org/10540057/diff/1/ppapi/proxy/ppb_flash_proxy.cc
File ppapi/proxy/ppb_flash_proxy.cc (right):

http://codereview.chromium.org/10540057/diff/1/ppapi/proxy/ppb_flash_proxy.cc...
ppapi/proxy/ppb_flash_proxy.cc:8: #include <set>
Remove includes that are not needed any more. For example, this one and
ipc_channel_proxy.h. (I didn't go through the whole list. There might be
others.)

http://codereview.chromium.org/10540057/diff/1/ppapi/thunk/ppb_flash_file_mod...
File ppapi/thunk/ppb_flash_file_modulelocal_thunk.cc (right):

http://codereview.chromium.org/10540057/diff/1/ppapi/thunk/ppb_flash_file_mod...
ppapi/thunk/ppb_flash_file_modulelocal_thunk.cc:18: return true;
This is not true for in-process mode, right?
So we should probably keep the corresponding method in _api, and then return
different values from the proxy and the impl.

[Tom Sepez, 2012-06-07 20:07:20]

Hey Brett, could I get a quick review.  Thanks.

[brettw, 2012-06-07 21:48:07]

Yuzhu is also an owner so he should be able to finish this (since he already
started looking).

[yzshen1, 2012-06-07 23:27:06]

https://chromiumcodereview.appspot.com/10540057/diff/1005/ppapi/proxy/ppb_fla...
File ppapi/proxy/ppb_flash_proxy.cc (right):

https://chromiumcodereview.appspot.com/10540057/diff/1005/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_proxy.cc:373: ppapi::PepperFilePath
pepper_path(ppapi::PepperFilePath::DOMAIN_MODULE_LOCAL,
One more thing that needs to be changed: This is not the correct path. The
module name is missing!

In ppb_flash_impl.cc, we have something like
PepperFilePath::MakeModuleLocal(plugin_instance->module()->name(), path);

And also note that we couldn't access plugin instance here, because this may be
accessed from another thread.

[Tom Sepez, 2012-06-07 23:31:21]

https://chromiumcodereview.appspot.com/10540057/diff/1005/ppapi/proxy/ppb_fla...
File ppapi/proxy/ppb_flash_proxy.cc (right):

https://chromiumcodereview.appspot.com/10540057/diff/1005/ppapi/proxy/ppb_fla...
ppapi/proxy/ppb_flash_proxy.cc:373: ppapi::PepperFilePath
pepper_path(ppapi::PepperFilePath::DOMAIN_MODULE_LOCAL,
On 2012/06/07 23:27:06, yzshen1 wrote:
> One more thing that needs to be changed: This is not the correct path. The
> module name is missing!
> 

One of the changes I made in the a previous CL was to have the browser supply
the name of the module, since we can't trust the renderer or the plugin not to
lie about it when they are compromised.

I need to go back and look at what happens when the renderer makes the
equivalent call -- whether we get a double insertion of the module name.

[Tom Sepez, 2012-06-07 23:36:12]

> what happens when the renderer makes the equivalent call -- whether we get a
double insertion of the module name.

No, the renderer is only allowed to open absolute paths to which it has been
explicitly granted permissions.  To open these module local paths, we need the
out-of-process channel.

[yzshen1, 2012-06-07 23:36:31]

On 2012/06/07 23:31:21, Tom Sepez wrote:
>
https://chromiumcodereview.appspot.com/10540057/diff/1005/ppapi/proxy/ppb_fla...
> File ppapi/proxy/ppb_flash_proxy.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10540057/diff/1005/ppapi/proxy/ppb_fla...
> ppapi/proxy/ppb_flash_proxy.cc:373: ppapi::PepperFilePath
> pepper_path(ppapi::PepperFilePath::DOMAIN_MODULE_LOCAL,
> On 2012/06/07 23:27:06, yzshen1 wrote:
> > One more thing that needs to be changed: This is not the correct path. The
> > module name is missing!
> > 
> 
> One of the changes I made in the a previous CL was to have the browser supply
> the name of the module, since we can't trust the renderer or the plugin not to
> lie about it when they are compromised.
This makes good sense.

> 
> I need to go back and look at what happens when the renderer makes the
> equivalent call -- whether we get a double insertion of the module name.

Thanks!

[yzshen1, 2012-06-07 23:38:47]

On 2012/06/07 23:36:12, Tom Sepez wrote:
> > what happens when the renderer makes the equivalent call -- whether we get a
> double insertion of the module name.
> 
> No, the renderer is only allowed to open absolute paths to which it has been
> explicitly granted permissions.  To open these module local paths, we need the
> out-of-process channel.

I don't understand: in ppb_flash_impl, methods like OpenFile() deal with module
local path.
Have I misunderstood anything?

[Tom Sepez, 2012-06-07 23:42:55]

We kept the in-process paths intact for the OpenFileRef case which uses much of
the same code in the delegate.  But should an openfile message come from the
renderer, specifying a module-local path, it gets an error back from the
browser.  

I've put the decision about what paths are acceptable for which process up to
the browser.  So there's some code that won't work in-process.

[yzshen1, 2012-06-07 23:45:15]

On 2012/06/07 23:42:55, Tom Sepez wrote:
> We kept the in-process paths intact for the OpenFileRef case which uses much
of
> the same code in the delegate.  But should an openfile message come from the
> renderer, specifying a module-local path, it gets an error back from the
> browser.  
> 
> I've put the decision about what paths are acceptable for which process up to
> the browser.  So there's some code that won't work in-process.

Okay. I understand that the in-process OpenFile/etc. won't work because that
they use module-local paths which are not allowed.
But why don't we remove the implementation entirely?

[Tom Sepez, 2012-06-07 23:48:54]

> Okay. I understand that the in-process OpenFile/etc. won't work because that
> they use module-local paths which are not allowed.
> But why don't we remove the implementation entirely?

I've thought about that, and it may happen as part of a later cleanup, but this
leaves open the possibility of the browser implementing an equivalent policy for
in-process case down the road.

For now, we need to segregate out the plugin from the renderer to prevent a
corrupt renderer from writing corrupt data on top of the out of process plugin.

But there's no reason that there couldn't be a separate directory for the
in-process cases.

[yzshen1, 2012-06-07 23:50:25]

On 2012/06/07 23:48:54, Tom Sepez wrote:
> > Okay. I understand that the in-process OpenFile/etc. won't work because that
> > they use module-local paths which are not allowed.
> > But why don't we remove the implementation entirely?
> 
> I've thought about that, and it may happen as part of a later cleanup, but
this
> leaves open the possibility of the browser implementing an equivalent policy
for
> in-process case down the road.
> 
> For now, we need to segregate out the plugin from the renderer to prevent a
> corrupt renderer from writing corrupt data on top of the out of process
plugin.
> 
> But there's no reason that there couldn't be a separate directory for the
> in-process cases.

Okay. Thanks for explanation!

[yzshen1, 2012-06-07 23:53:15]

LGTM except that you haven't remove unnecessary includes.


On 2012/06/07 23:50:25, yzshen1 wrote:
> On 2012/06/07 23:48:54, Tom Sepez wrote:
> > > Okay. I understand that the in-process OpenFile/etc. won't work because
that
> > > they use module-local paths which are not allowed.
> > > But why don't we remove the implementation entirely?
> > 
> > I've thought about that, and it may happen as part of a later cleanup, but
> this
> > leaves open the possibility of the browser implementing an equivalent policy
> for
> > in-process case down the road.
> > 
> > For now, we need to segregate out the plugin from the renderer to prevent a
> > corrupt renderer from writing corrupt data on top of the out of process
> plugin.
> > 
> > But there's no reason that there couldn't be a separate directory for the
> > in-process cases.
> 
> Okay. Thanks for explanation!

[commit-bot: I haz the power, 2012-06-08 16:42:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/10540057/13003

[commit-bot: I haz the power, 2012-06-08 17:56:28]

Change committed as 141236