PPAPI: Make V8VarConverter longer-lived

PPAPI: Make V8VarConverter longer-lived

For supporting sending browser-hosted resources (like FileSystem and FileRef) in postMessageAndAwaitResponse, the ResourceConverter is going to need to save some state across conversions. Therefore, MessageChannel will have to use one V8VarConverter instead of using it entirely on the stack. This also means PepperTryCatch* needs to be passed a V8VarConverter instead of making its own.

BUG=417316
Committed: https://crrev.com/e1705472bf0bbcace45a16dcc66f795f974a3a02
Cr-Commit-Position: refs/heads/master@{#299170}

[dmichael (off chromium), 2014-10-08 22:48:23]

dmichael@chromium.org changed reviewers:
+ raymes@chromium.org

[dmichael (off chromium), 2014-10-08 22:48:35]

https://codereview.chromium.org/635593004/diff/20001/content/renderer/pepper/...
File content/renderer/pepper/resource_converter.cc (right):

https://codereview.chromium.org/635593004/diff/20001/content/renderer/pepper/...
content/renderer/pepper/resource_converter.cc:215: RendererPpapiHost* host =
RendererPpapiHost::GetForPPInstance(instance_);
I moved the host lookup inside, because for MessageChannel, it was getting
looked up too early and being bound with the trusted in-process NaCl plugin's
RendererPpapiHost instead of the out-of-process/untrusted one.

There might be better ways of doing this.

[raymes, 2014-10-09 22:04:42]

Just a few small comments. Could you elaborate on what state needs to be kept
across conversions? How would a user of the class know whether to keep it alive
across conversions? This may become clearer in the followup CLs :)

https://codereview.chromium.org/635593004/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/message_channel.cc (right):

https://codereview.chromium.org/635593004/diff/40001/content/renderer/pepper/...
content/renderer/pepper/message_channel.cc:18: #include
"content/renderer/pepper/v8_var_converter.h"
nit: I guess we can remove this.

https://codereview.chromium.org/635593004/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.cc (right):

https://codereview.chromium.org/635593004/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:25: : instance_(instance),
var_converter_(var_converter) {}
nit, optional: since we know that the conversion mode should always be
V8VarConverter::AllowObjectVars here, maybe we could DCHECK it?

https://codereview.chromium.org/635593004/diff/40001/ppapi/host/ppapi_host.cc
File ppapi/host/ppapi_host.cc (right):

https://codereview.chromium.org/635593004/diff/40001/ppapi/host/ppapi_host.cc...
ppapi/host/ppapi_host.cc:256: //NOTREACHED();
Was this getting hit?

[dmichael (off chromium), 2014-10-09 22:45:37]

Thanks for the review!

I think the state that's saved will be clearer in the next CL. I'm going to need
a way to synchronously flush a ResourceConverter. The big problem with that is
that there might be asynchronous conversions still pending. When we do a sync
flush, we need to not only get the results for the sync conversion, but any
results for *prior* async conversions that we didn't receive a response for yet.
We need to be able to associate those with the resource hosts, which currently
are not really something we can look up, because they're locked in bound
callbacks.

https://codereview.chromium.org/635593004/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/message_channel.cc (right):

https://codereview.chromium.org/635593004/diff/40001/content/renderer/pepper/...
content/renderer/pepper/message_channel.cc:18: #include
"content/renderer/pepper/v8_var_converter.h"
On 2014/10/09 22:04:42, raymes wrote:
> nit: I guess we can remove this.

Done.

https://codereview.chromium.org/635593004/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.cc (right):

https://codereview.chromium.org/635593004/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:25: : instance_(instance),
var_converter_(var_converter) {}
On 2014/10/09 22:04:42, raymes wrote:
> nit, optional: since we know that the conversion mode should always be
> V8VarConverter::AllowObjectVars here, maybe we could DCHECK it?
You mean kAllowObjectVars? We don't know that here...  MessageChannel passes
kDisallowObjectVars to PepperTryCatchV8, which ends up here.

We could in theory DCHECK in PepperTryCatchVar (maybe that's what you meant?).
But I think that's only because we don't happen to use Disallow for
PepperTryCatchVar. But I don't think there's any reason we couldn't do that if
we wanted to.

https://codereview.chromium.org/635593004/diff/40001/ppapi/host/ppapi_host.cc
File ppapi/host/ppapi_host.cc (right):

https://codereview.chromium.org/635593004/diff/40001/ppapi/host/ppapi_host.cc...
ppapi/host/ppapi_host.cc:256: //NOTREACHED();
On 2014/10/09 22:04:42, raymes wrote:
> Was this getting hit?

Ah, good catch. It *was* getting hit; that was the symptom of the problem with
binding the PepperRendererHostImpl too early.

It was leftover from debugging; removed in the latest patch set.

[raymes, 2014-10-10 19:27:14]

lgtm

https://codereview.chromium.org/635593004/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.cc (right):

https://codereview.chromium.org/635593004/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:25: : instance_(instance),
var_converter_(var_converter) {}
On 2014/10/09 22:45:37, dmichael wrote:
> On 2014/10/09 22:04:42, raymes wrote:
> > nit, optional: since we know that the conversion mode should always be
> > V8VarConverter::AllowObjectVars here, maybe we could DCHECK it?
> You mean kAllowObjectVars? We don't know that here...  MessageChannel passes
> kDisallowObjectVars to PepperTryCatchV8, which ends up here.
> 
> We could in theory DCHECK in PepperTryCatchVar (maybe that's what you meant?).

Sorry, yeah that's what I meant :)

> But I think that's only because we don't happen to use Disallow for
> PepperTryCatchVar. But I don't think there's any reason we couldn't do that if
> we wanted to.

Ok, sounds good.

[dmichael (off chromium), 2014-10-10 19:33:39]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-10-10 19:35:03]

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

[commit-bot: I haz the power, 2014-10-10 20:03:27]

Committed patchset #5 (id:170001)

[commit-bot: I haz the power, 2014-10-10 20:04:19]

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