Open pepper files directly in browser.

Open pepper files directly in browser.

This CL merges in the changes for the per-profile plugin process from the
previously retired CL.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=140093
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=140399
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=140602

[Tom Sepez, 2012-05-23 20:39:25]

Brett, PTAL.

This is a less disruptive version of what we'd been discussing, the main
difference being that the in-process plugin file messages are still present, but
they aren't honored by the browser if they exceed the privileges granted to the
ordinary renderer.

This keeps the out-of-process OpenFileRef() and QueryFileRef() paths intact
although subject to the restriction above.

[brettw, 2012-05-29 18:14:15]

Sorry I took so long

http://codereview.chromium.org/10387195/diff/27027/content/browser/renderer_h...
File content/browser/renderer_host/pepper_file_message_filter.cc (right):

http://codereview.chromium.org/10387195/diff/27027/content/browser/renderer_h...
content/browser/renderer_host/pepper_file_message_filter.cc:226: if
(pepper_path.path().IsAbsolute() &&
Indent 2 less spaces.

http://codereview.chromium.org/10387195/diff/27027/content/browser/renderer_h...
content/browser/renderer_host/pepper_file_message_filter.cc:238: :
PepperFileMessageFilter(child_id) {
Colon should be indented 4 spaces total.

http://codereview.chromium.org/10387195/diff/27027/content/ppapi_plugin/ppapi...
File content/ppapi_plugin/ppapi_thread.cc (right):

http://codereview.chromium.org/10387195/diff/27027/content/ppapi_plugin/ppapi...
content/ppapi_plugin/ppapi_thread.cc:212: base::PlatformFileError error =
base::PLATFORM_FILE_ERROR_FAILED;
Is it possible to put all of this message construction stuff in the proxy rather
than in here? I don't see any security reason why the proxy can't generate the
messages.

It may be necessary to move or have a different PepperFilePath in shared_impl or
something so that it can be shared with the browser & proxy.

[Tom Sepez, 2012-05-30 20:21:36]

Brett, 
This latest CL moves the PepperFilePath from webkit/ppapi to ppapi/shared_impl,
moves pepper_file_messages to ppapi/proxy, and deals with the consequences.  Let
me know if I've violated some layering.

[brettw, 2012-05-31 22:36:11]

LGTM.

None of the flash file I/O functions will work in process anymore, right? Can we
just remove all the impl and the plugin delegate functions? It seems like a lot
of code. If this sounds possible, maybe it's best to do in a followup patch to
avoid delaying this one more.

[Tom Sepez, 2012-05-31 22:42:36]

These will work in-process so long as there is explicit permission for the
renderer to open the file.  There are two cases, queryfileref and openfileref,
that go through the same delegate methods.  These continue to work for us
because these are used on the results of file chooser (etc) so that permission
has been granted (to an absolute path).

I suppose 4 of the delegate methods could go; but this design forces the
decision about what is allowed up to the browser, where I think it belongs.

[brettw, 2012-05-31 22:43:40]

okay

[commit-bot: I haz the power, 2012-06-01 19:52:37]

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

[commit-bot: I haz the power, 2012-06-01 19:53:44]

Failed to apply patch for ppapi/proxy/pepper_file_messages.cc:
While running patch -p0 --forward --force;
patching file ppapi/proxy/pepper_file_messages.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
ppapi/proxy/pepper_file_messages.cc.rej

[Tom Sepez, 2012-06-01 22:04:33]

Open pepper files directly in browser (take 2).

[commit-bot: I haz the power, 2012-06-04 19:14:28]

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

[commit-bot: I haz the power, 2012-06-04 19:14:54]

Failed to apply patch for ppapi/proxy/pepper_file_messages.cc:
While running patch -p0 --forward --force;
patching file ppapi/proxy/pepper_file_messages.cc
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file
ppapi/proxy/pepper_file_messages.cc.rej

[Tom Sepez, 2012-06-04 23:01:07]

Open the pepper files directly in the browser.

[yzshen1, 2012-06-06 16:56:37]

https://chromiumcodereview.appspot.com/10387195/diff/96001/ppapi/proxy/ppb_fl...
File ppapi/proxy/ppb_flash_proxy.cc (right):

https://chromiumcodereview.appspot.com/10387195/diff/96001/ppapi/proxy/ppb_fl...
ppapi/proxy/ppb_flash_proxy.cc:663: g_module_local_thread_adapter = new
ModuleLocalThreadAdapter();
The code no longer uses this. You could make this and
ClearThreadAdapterForInstance no-ops. And remove the definition of
ModuleLocalThreadAdapter.

https://chromiumcodereview.appspot.com/10387195/diff/96001/ppapi/proxy/ppb_fl...
ppapi/proxy/ppb_flash_proxy.cc:696: if
(PluginGlobals::Get()->plugin_proxy_delegate()->SendToBrowser(
I don't think it is safe to do so. OpenFile and other methods could be called
from non-main thread.

SendToBrowser() calls into ChildThread::Send() and it is not intended to be used
by multiple threads.

Have I missed anything?

[Tom Sepez, 2012-06-06 17:07:57]

So should we make SendToBrowser() post a task to the appropriate thread?  Or do
you think that we should fix each of the 6 or so new places that calls it to do
the same?

[yzshen1, 2012-06-06 17:13:49]

On 2012/06/06 17:07:57, Tom Sepez wrote:
> So should we make SendToBrowser() post a task to the appropriate thread?  Or
do
We cannot do post task here. Because these are all synchronous messages.

The thread adapter in this file serves the purpose of sending sync messages from
non-main threads. You could take a look at how to make modification to it.

Or you could consider IPC::SyncMessageFilter. (I actually got advice from people
that we should replace the thread adapter with IPC::SyncMessageFilter.

> you think that we should fix each of the 6 or so new places that calls it to
do
> the same?

[Tom Sepez, 2012-06-06 23:04:30]

Is more required than changing PpapiThread::SendToBrowser() along the lines of:


 bool PpapiThread::SendToBrowser(IPC::Message* msg) {
-  return Send(msg);
+  if (MessageLoop::current() == message_loop())
+    return ChildThread::Send(msg);
+
+  return sync_message_filter()->Send(msg);
 }

[brettw, 2012-06-06 23:06:29]

I *think* that will work.

[yzshen, 2012-06-06 23:28:37]

(Somehow my reply disappeared.)

Previously, PpapiGlobals and PpapiThread are expected to be used on a
single thread. We may want to add comments about threading for them.

On Wed, Jun 6, 2012 at 4:25 PM, <yzshen@chromium.org> wrote:

>
http://codereview.chromium.**org/10387195/<http://codereview.chromium.org/103...
>



-- 
Best regards,
Yuzhu Shen.