[PPAPI] Added PP_VARTYPE_RESOURCE as a PP_VarType enum value.

[PPAPI] Added PP_VARTYPE_RESOURCE as a PP_VarType enum value.

This type is not yet useful. There is currently no way to generate one
of these. If the plugin manually builds one and sends it in a message,
it will currently check-fail, crashing the plugin.

BUG=177017
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222759

[Matt Giuca, 2013-09-03 08:32:22]

Hi guys,

This is the first change for the Native Client file passing that actually
affects the API. I talked with Raymes about whether you'd want
PP_VARTYPE_RESOURCE in the API when it doesn't actually do anything, but I think
it's helpful to split up these CLs, and there is precedent in that ARRAY and
DICTIONARY were there for many releases even though they did nothing.

https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data.cc
File ppapi/proxy/raw_var_data.cc (right):

https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:248: CHECK(false) << "Serializing
PP_VARTYPE_RESOURCE is not supported.";
I agonized about whether to check-fail here or have a more graceful error (e.g.,
log error and cancel the message post). I could do the latter, but it requires
some refactoring so it's probably not worth it for a temporary change.

However, there is an ongoing discussion with Yuzhu about how to deal with
resources we can't convert to V8 values (later on), and if we are going to make
it cancel sending the whole message, then we may as well do the refactor here.

I'd be interested in whether you think this checkfail is OK for a temporary
solution.

[Matt Giuca, 2013-09-03 08:36:31]

https://codereview.chromium.org/18599005/diff/43001/ppapi/api/pp_var.idl
File ppapi/api/pp_var.idl (right):

https://codereview.chromium.org/18599005/diff/43001/ppapi/api/pp_var.idl#newc...
ppapi/api/pp_var.idl:58: * Arrays and dictionaries are not currently supported
but will be added
FYI I have a CL out to fix this comment.
(https://codereview.chromium.org/23757014/)

[dmichael (off chromium), 2013-09-03 16:25:17]

https://codereview.chromium.org/18599005/diff/43001/content/renderer/pepper/m...
File content/renderer/pepper/message_channel.cc (right):

https://codereview.chromium.org/18599005/diff/43001/content/renderer/pepper/m...
content/renderer/pepper/message_channel.cc:108: // Objects/Arrays/Dictionaries
not supported by PostMessage in-process.
Maybe update the comment to just say "These types are not"

(And I think there's no need to add support for RESOURCE in process)

https://codereview.chromium.org/18599005/diff/43001/content/renderer/pepper/v...
File content/renderer/pepper/v8_var_converter.cc (right):

https://codereview.chromium.org/18599005/diff/43001/content/renderer/pepper/v...
content/renderer/pepper/v8_var_converter.cc:151: case PP_VARTYPE_RESOURCE:
It would be nice to add a TODO and reference the bug here.

https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data.cc
File ppapi/proxy/raw_var_data.cc (right):

https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:248: CHECK(false) << "Serializing
PP_VARTYPE_RESOURCE is not supported.";
On 2013/09/03 08:32:22, Matt Giuca wrote:
> I agonized about whether to check-fail here or have a more graceful error
(e.g.,
> log error and cancel the message post). I could do the latter, but it requires
> some refactoring so it's probably not worth it for a temporary change.
> 
> However, there is an ongoing discussion with Yuzhu about how to deal with
> resources we can't convert to V8 values (later on), and if we are going to
make
> it cancel sending the whole message, then we may as well do the refactor here.
> 
> I'd be interested in whether you think this checkfail is OK for a temporary
> solution.
The strategy I think we were going to use for other failures in serialization
(where it's not easy to bubble up an error to the sender) is to create an
"Invalid" SerializedVar. The receiving end can then decide what to do when it
gets an invalid var. In Messaging, we just drop it and maybe log. You'd have to
go to all the call sites for Create() and make sure the NULL bubbles up right
(it looks like it doesn't right now).

I think it might be worth doing that...  in case the resource var is not all
done in 31. WDYT?

[yzshen1, 2013-09-03 20:19:53]

https://codereview.chromium.org/18599005/diff/43001/ppapi/api/pp_var.idl
File ppapi/api/pp_var.idl (right):

https://codereview.chromium.org/18599005/diff/43001/ppapi/api/pp_var.idl#newc...
ppapi/api/pp_var.idl:112: * <code>PP_VARTYPE_OBJECT</code>,
<code>PP_VARTYPE_ARRAY</code>,
Please also add PP_VARTYPE_ARRAY_BUFFER here.

https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data.cc
File ppapi/proxy/raw_var_data.cc (right):

https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:248: CHECK(false) << "Serializing
PP_VARTYPE_RESOURCE is not supported.";
On 2013/09/03 08:32:22, Matt Giuca wrote:
> I agonized about whether to check-fail here or have a more graceful error
(e.g.,
> log error and cancel the message post). I could do the latter, but it requires
> some refactoring so it's probably not worth it for a temporary change.
> 
> However, there is an ongoing discussion with Yuzhu about how to deal with
> resources we can't convert to V8 values (later on), and if we are going to
make
> it cancel sending the whole message, then we may as well do the refactor here.
> 
> I'd be interested in whether you think this checkfail is OK for a temporary
> solution.

WRT our discussion, I personally prefer not to give an incomplete message to JS
when failure occurs. If we send some resource to the renderer side and find out
that it cannot be converted to V8 value. We could drop the message at that point
(and log errors). We don't have to make all decisions at the plugin side.

[dmichael (off chromium), 2013-09-03 20:39:41]

https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data.cc
File ppapi/proxy/raw_var_data.cc (right):

https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:248: CHECK(false) << "Serializing
PP_VARTYPE_RESOURCE is not supported.";
On 2013/09/03 20:19:53, yzshen1 wrote:
> On 2013/09/03 08:32:22, Matt Giuca wrote:
> > I agonized about whether to check-fail here or have a more graceful error
> (e.g.,
> > log error and cancel the message post). I could do the latter, but it
requires
> > some refactoring so it's probably not worth it for a temporary change.
> > 
> > However, there is an ongoing discussion with Yuzhu about how to deal with
> > resources we can't convert to V8 values (later on), and if we are going to
> make
> > it cancel sending the whole message, then we may as well do the refactor
here.
> > 
> > I'd be interested in whether you think this checkfail is OK for a temporary
> > solution.
> 
> WRT our discussion, I personally prefer not to give an incomplete message to
JS
> when failure occurs. If we send some resource to the renderer side and find
out
> that it cannot be converted to V8 value. We could drop the message at that
point
> (and log errors). We don't have to make all decisions at the plugin side.
I agree; I think we're suggesting the same thing :). We can make an invalid
serialized var:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppb_in...
And the Messaging proxy should do the right thing already.

[yzshen1, 2013-09-03 20:59:26]

On 2013/09/03 20:39:41, dmichael wrote:
>
https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data.cc
> File ppapi/proxy/raw_var_data.cc (right):
> 
>
https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data....
> ppapi/proxy/raw_var_data.cc:248: CHECK(false) << "Serializing
> PP_VARTYPE_RESOURCE is not supported.";
> On 2013/09/03 20:19:53, yzshen1 wrote:
> > On 2013/09/03 08:32:22, Matt Giuca wrote:
> > > I agonized about whether to check-fail here or have a more graceful error
> > (e.g.,
> > > log error and cancel the message post). I could do the latter, but it
> requires
> > > some refactoring so it's probably not worth it for a temporary change.
> > > 
> > > However, there is an ongoing discussion with Yuzhu about how to deal with
> > > resources we can't convert to V8 values (later on), and if we are going to
> > make
> > > it cancel sending the whole message, then we may as well do the refactor
> here.
> > > 
> > > I'd be interested in whether you think this checkfail is OK for a
temporary
> > > solution.
> > 
> > WRT our discussion, I personally prefer not to give an incomplete message to
> JS
> > when failure occurs. If we send some resource to the renderer side and find
> out
> > that it cannot be converted to V8 value. We could drop the message at that
> point
> > (and log errors). We don't have to make all decisions at the plugin side.
> I agree; I think we're suggesting the same thing :). We can make an invalid
> serialized var:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/ppb_in...
> And the Messaging proxy should do the right thing already.

Right. :) I agree with you that the plugin side could send an invalid serialized
var in case of failure and let the renderer-side handle it.

In addition to that, it is possible that a resource var can be successfully sent
to the renderer side, but cannot be converted to a V8 object. (It is something
that we don't support in JS, for example.) In that case, the MessageChannel will
know about the failure and log an error.

[Matt Giuca, 2013-09-10 01:17:13]

https://codereview.chromium.org/18599005/diff/43001/content/renderer/pepper/m...
File content/renderer/pepper/message_channel.cc (right):

https://codereview.chromium.org/18599005/diff/43001/content/renderer/pepper/m...
content/renderer/pepper/message_channel.cc:108: // Objects/Arrays/Dictionaries
not supported by PostMessage in-process.
On 2013/09/03 16:25:17, dmichael wrote:
> Maybe update the comment to just say "These types are not"
> 
> (And I think there's no need to add support for RESOURCE in process)

Done.

(We'll have to see about RESOURCE in process later. I don't seem to have needed
this so far, and I don't really understand what it is used for.)

https://codereview.chromium.org/18599005/diff/43001/content/renderer/pepper/v...
File content/renderer/pepper/v8_var_converter.cc (right):

https://codereview.chromium.org/18599005/diff/43001/content/renderer/pepper/v...
content/renderer/pepper/v8_var_converter.cc:151: case PP_VARTYPE_RESOURCE:
On 2013/09/03 16:25:17, dmichael wrote:
> It would be nice to add a TODO and reference the bug here.

Done.

https://codereview.chromium.org/18599005/diff/43001/ppapi/api/pp_var.idl
File ppapi/api/pp_var.idl (right):

https://codereview.chromium.org/18599005/diff/43001/ppapi/api/pp_var.idl#newc...
ppapi/api/pp_var.idl:58: * Arrays and dictionaries are not currently supported
but will be added
On 2013/09/03 08:36:31, Matt Giuca wrote:
> FYI I have a CL out to fix this comment.
> (https://codereview.chromium.org/23757014/)

Done.

https://codereview.chromium.org/18599005/diff/43001/ppapi/api/pp_var.idl#newc...
ppapi/api/pp_var.idl:112: * <code>PP_VARTYPE_OBJECT</code>,
<code>PP_VARTYPE_ARRAY</code>,
On 2013/09/03 20:19:53, yzshen1 wrote:
> Please also add PP_VARTYPE_ARRAY_BUFFER here.

Done.

https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data.cc
File ppapi/proxy/raw_var_data.cc (right):

https://codereview.chromium.org/18599005/diff/43001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:248: CHECK(false) << "Serializing
PP_VARTYPE_RESOURCE is not supported.";
> I think it might be worth doing that...  in case the resource
> var is not all done in 31. WDYT?

So this is a lot more temporary than I let on. I have another CL almost ready to
go, that adds ResourceRawVarData, which will remove this NOTIMPLEMENTED(). That
should definitely be in M31 even if the rest of resource var handling is not.

After that CL, if someone tries to pass a ResourceRawVarData from JS to NaCl, it
will successfully pass it through to the renderer and hit v8_var_converter (and
I'll have to write logic in there to make it fail to produce a V8 value).

Therefore, I don't think it is worth making this not checkfail here. However,
beause I'm in a hurry, I did implement it in Patch Set 8. I'd prefer to submit
Patch Set 7, so please review that. But if you want it to fail gracefully
instead of checkfailing in this CL, please look at Patch Set 8.

[Matt Giuca, 2013-09-10 22:45:51]

It's getting really complicated to manage this patch.

PS 9 is based on PS 7 -- this is the one that just checkfails without the big
refactor.
PS 8 is the one that has been refactored so that it doesn't checkfail. (I'm
hoping I don't need to submit PS 8, because I will very shortly remove the need
for this refactor.)

[dmichael (off chromium), 2013-09-11 18:30:05]

lgtm

https://codereview.chromium.org/18599005/diff/74013/ppapi/shared_impl/unittes...
File ppapi/shared_impl/unittest_utils.cc (right):

https://codereview.chromium.org/18599005/diff/74013/ppapi/shared_impl/unittes...
ppapi/shared_impl/unittest_utils.cc:182: expected_message.size()) != 0) {
Why not just memcmp the data that is expected to be equal, rather than mutating
actual_message?
I.e.:
DCHECK(expected_message.size() >= 3);
memcmp(expected_message.data()+3, actual_message.data()+3,
       expected_message.size()-3)

[raymes, 2013-09-11 22:52:28]

lgtm

https://chromiumcodereview.appspot.com/18599005/diff/74013/ppapi/api/pp_var.idl
File ppapi/api/pp_var.idl (right):

https://chromiumcodereview.appspot.com/18599005/diff/74013/ppapi/api/pp_var.i...
ppapi/api/pp_var.idl:88: * revisions. These objects are reference counted, so
AddRef and Release must
future revisions -> the future

https://chromiumcodereview.appspot.com/18599005/diff/74013/ppapi/proxy/raw_va...
File ppapi/proxy/raw_var_data.cc (right):

https://chromiumcodereview.appspot.com/18599005/diff/74013/ppapi/proxy/raw_va...
ppapi/proxy/raw_var_data.cc:248: CHECK(false) << "Serializing
PP_VARTYPE_RESOURCE is not supported.";
I realise that you are going to be changing this soon, but I think it might be
better to maintain the existing behavior here and just have a NOTREACHED() here.

https://chromiumcodereview.appspot.com/18599005/diff/74013/ppapi/shared_impl/...
File ppapi/shared_impl/var.cc (right):

https://chromiumcodereview.appspot.com/18599005/diff/74013/ppapi/shared_impl/...
ppapi/shared_impl/var.cc:74: } else if (resource->creation_message().type() !=
0) {
This makes me wonder whether maybe we should have an enum for specifying the
type of the resource var (i.e. either pending or existing). I'm ok with leaving
it as-is for now though.

[yzshen1, 2013-09-11 22:57:10]

LGTM

https://codereview.chromium.org/18599005/diff/74013/ppapi/shared_impl/var.cc
File ppapi/shared_impl/var.cc (right):

https://codereview.chromium.org/18599005/diff/74013/ppapi/shared_impl/var.cc#...
ppapi/shared_impl/var.cc:74: } else if (resource->creation_message().type() !=
0) {
Or maybe we could have some boolean method such as ResourceVar::IsPending()?

On 2013/09/11 22:52:29, raymes wrote:
> This makes me wonder whether maybe we should have an enum for specifying the
> type of the resource var (i.e. either pending or existing). I'm ok with
leaving
> it as-is for now though.

[Matt Giuca, 2013-09-12 07:08:24]

https://codereview.chromium.org/18599005/diff/74013/ppapi/api/pp_var.idl
File ppapi/api/pp_var.idl (right):

https://codereview.chromium.org/18599005/diff/74013/ppapi/api/pp_var.idl#newc...
ppapi/api/pp_var.idl:88: * revisions. These objects are reference counted, so
AddRef and Release must
On 2013/09/11 22:52:29, raymes wrote:
> future revisions -> the future

Done. (Haha, note that I copied this text from the text that used to be on ARRAY
and DICTIONARY.)

https://codereview.chromium.org/18599005/diff/74013/ppapi/proxy/raw_var_data.cc
File ppapi/proxy/raw_var_data.cc (right):

https://codereview.chromium.org/18599005/diff/74013/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:248: CHECK(false) << "Serializing
PP_VARTYPE_RESOURCE is not supported.";
OK, fair enough.

That will result in a NULL pointer dereference in the plugin module on the
release build. But I suppose that a) that is equivalent to the current behaviour
anyway, if the user decides to set a var's type to 100, and b) that is no worse
than a checkfail (just harder to diagnose).

https://codereview.chromium.org/18599005/diff/74013/ppapi/shared_impl/unittes...
File ppapi/shared_impl/unittest_utils.cc (right):

https://codereview.chromium.org/18599005/diff/74013/ppapi/shared_impl/unittes...
ppapi/shared_impl/unittest_utils.cc:182: expected_message.size()) != 0) {
That would assume implementation details about the message format (i.e., that
the flags are stored in a certain place in the header, that there is a certain
endianness, the size of all fields preceding the flags, the order the compiler
chooses to place them in the struct). Note that it wouldn't be 3 bytes to skip,
but I believe 32. You would also then ignore all the information in the other
header fields, including the message type.

It is much safer to stomp out the field using the interface provided (although
even here I am assuming the implementation detail that the reference number is
in the top 24 bits; but I don't have much choice). Note that I am not mutating
the input message -- I make a copy of it on line 164 so this mutation doesn't
leak any side effects.

https://codereview.chromium.org/18599005/diff/74013/ppapi/shared_impl/var.cc
File ppapi/shared_impl/var.cc (right):

https://codereview.chromium.org/18599005/diff/74013/ppapi/shared_impl/var.cc#...
ppapi/shared_impl/var.cc:74: } else if (resource->creation_message().type() !=
0) {
Yeah OK. I think I'll add IsPending (it doesn't seem worth having an enum, since
it's really only used for this debug string), but I'll do it in another CL since
I want to submit this one without another review cycle.

[commit-bot: I haz the power, 2013-09-12 07:08:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/18599005/100001

[commit-bot: I haz the power, 2013-09-12 11:11:23]

Change committed as 222759