Replace NPObject usage in ppapi with gin

Replace NPObject usage in ppapi with gin

This replaces usage of NPObject in pepper with gin-backed V8 objects. It is unfortunate that this CL is so large, but there isn't a nice way to have the old implementation and the new one side-by-side. 

There are 4 major parts to this CL:
1) Changing the HostVarTracker to track V8ObjectVars rather than NPObjectVars (host_var_tracker.cc).
2) Changing plugin elements (in plugin_object.cc) to be gin-backed objects. 
3) Changing postMessage bindings (message_channel.cc) be gin-backed objects.
4) Changing the implementation of PPB_Var_Deprecated (ppb_var_deprecated_impl.cc) to call directly into V8.


BUG=351636
Committed: https://crrev.com/2515b7dc2832404db4aa8d7d20257f0865c0d931
Cr-Commit-Position: refs/heads/master@{#293366}

[raymes, 2014-08-11 06:49:03]

Hi jochen, dmichael,

PTAL at this final part to the npobject->gin conversion. Unfortunately it's
large. I tried to split off as much as possible into the prior CLs which have
now landed, but this one is hard to break up.

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

https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
content/renderer/pepper/ppb_var_deprecated_impl.cc:255:
function.As<v8::Function>(), recv, argc, converted_args.get());
jochen: is this the appropriate way to execute a function in the frame context?

[jochen (gone - plz use gerrit), 2014-08-11 15:24:42]

On 2014/08/11 at 06:49:03, raymes wrote:
> Hi jochen, dmichael,
> 
> PTAL at this final part to the npobject->gin conversion. Unfortunately it's
large. I tried to split off as much as possible into the prior CLs which have
now landed, but this one is hard to break up.
> 
>
https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
> File content/renderer/pepper/ppb_var_deprecated_impl.cc (right):
> 
>
https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
> content/renderer/pepper/ppb_var_deprecated_impl.cc:255:
function.As<v8::Function>(), recv, argc, converted_args.get());
> jochen: is this the appropriate way to execute a function in the frame
context?

Yes, it is!

I will send more detailed comments tomorrow.

Good stuff!

[raymes, 2014-08-12 01:25:06]

On 2014/08/11 15:24:42, jochen (slow - soon OOO) wrote:
> On 2014/08/11 at 06:49:03, raymes wrote:
> > Hi jochen, dmichael,
> > 
> > PTAL at this final part to the npobject->gin conversion. Unfortunately it's
> large. I tried to split off as much as possible into the prior CLs which have
> now landed, but this one is hard to break up.
> > 
> >
>
https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
> > File content/renderer/pepper/ppb_var_deprecated_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
> > content/renderer/pepper/ppb_var_deprecated_impl.cc:255:
> function.As<v8::Function>(), recv, argc, converted_args.get());
> > jochen: is this the appropriate way to execute a function in the frame
> context?
> 
> Yes, it is!
> 
> I will send more detailed comments tomorrow.
> 
> Good stuff!

Great! I pulled out a few file deletions into a separate CL to make this a bit
smaller.

jochen@, I realize you won't have full context on this but please take a look at
the following files and do your best :)
-content/renderer/pepper/message_channel.h/cc
-content/renderer/pepper/pepper_plugin_instance_impl.h/cc
-content/renderer/pepper/pepper_webplugin_impl.h/cc
-content/renderer/pepper/plugin_object.h/cc
-content/renderer/pepper/ppb_var_deprecated_impl.cc

dmichael@ will review everything when he gets back in the office. 

Thanks!

[Krzysztof Olczyk, 2014-08-13 07:13:56]

https://codereview.chromium.org/459553003/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/459553003/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2376: return
ExecuteScript(pp_instance(), script.get(), NULL);
do you really need to execute the script here?

Can't you get WebLocalFrame like before,
and then wrap frame->mainWorldScriptContext()->Global() in PP_Var
with PepperTryCatchVar?

[jochen (gone - plz use gerrit), 2014-08-13 14:17:08]

some random notes.

I've uploaded a fix for gin that will hopefully let you work around the problem
with the interceptors

https://codereview.chromium.org/459553003/diff/40001/content/content_renderer...
File content/content_renderer.gypi (left):

https://codereview.chromium.org/459553003/diff/40001/content/content_renderer...
content/content_renderer.gypi:519: 'renderer/pepper/npobject_var.h',
don't you need to add v8object_var.* here?

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

https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
content/renderer/pepper/host_var_tracker_unittest.cc:33: MyObject() {
++g_v8objects_alive; }
nit. private:

also, disallow copy/assign

https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
content/renderer/pepper/host_var_tracker_unittest.cc:41:
PepperTryCatchForTest(PepperPluginInstanceImpl* instance)
explicit

https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
content/renderer/pepper/host_var_tracker_unittest.cc:54: };
disallow copy/assign

https://codereview.chromium.org/459553003/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/459553003/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:667: MessageChannel*
message_channel;
NULL initialize?

https://codereview.chromium.org/459553003/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1236: DCHECK(isolate ==
isolate_);
DCHECK_EQ

https://codereview.chromium.org/459553003/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2376: return
ExecuteScript(pp_instance(), script.get(), NULL);
+1

[raymes, 2014-08-14 00:47:57]

https://codereview.chromium.org/459553003/diff/40001/content/content_renderer...
File content/content_renderer.gypi (left):

https://codereview.chromium.org/459553003/diff/40001/content/content_renderer...
content/content_renderer.gypi:519: 'renderer/pepper/npobject_var.h',
They're already in the list :), they were added in a prior CL.

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

https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
content/renderer/pepper/host_var_tracker_unittest.cc:33: MyObject() {
++g_v8objects_alive; }
On 2014/08/13 14:17:07, jochen (slow - soon OOO) wrote:
> nit. private:
> 
> also, disallow copy/assign

Done.

https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
content/renderer/pepper/host_var_tracker_unittest.cc:41:
PepperTryCatchForTest(PepperPluginInstanceImpl* instance)
On 2014/08/13 14:17:07, jochen (slow - soon OOO) wrote:
> explicit

Done.

https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
content/renderer/pepper/host_var_tracker_unittest.cc:54: };
On 2014/08/13 14:17:07, jochen (slow - soon OOO) wrote:
> disallow copy/assign

Done.

https://codereview.chromium.org/459553003/diff/220001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/459553003/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:667: MessageChannel*
message_channel;
On 2014/08/13 14:17:07, jochen (slow - soon OOO) wrote:
> NULL initialize?

Done.

https://codereview.chromium.org/459553003/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1236: DCHECK(isolate ==
isolate_);
On 2014/08/13 14:17:07, jochen (slow - soon OOO) wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/459553003/diff/220001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2376: return
ExecuteScript(pp_instance(), script.get(), NULL);
On 2014/08/13 14:17:07, jochen (slow - soon OOO) wrote:
> +1

Thanks! I didn't know how to get at it, but this makes sense!

[dmichael (off chromium), 2014-08-20 23:09:16]

some comments, haven't made it too far yet

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

https://codereview.chromium.org/459553003/diff/40001/content/renderer/pepper/...
content/renderer/pepper/message_channel.h:53: static gin::WrapperInfo
kWrapperInfo;
Can it be made const?

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/host_var_tracker.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.cc:22: v8::Handle<v8::Object> object =
object_var->GetHandle();
Could you use v8::Local here instead of Handle to make it clearer?

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.cc:91: V8ObjectVarKey(instance,
v8::Handle<v8::Object>()));
suggestion: equal_range might be clearer and simplify the loop. (here and the
other places you use lower_bound)

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/host_var_tracker.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.h:45: // Returns 0 if the instance
isn't known.
not sure if it matters, but that means you can't tell whether the instance is
known and has zero vars, or is unknown.

You could use -1 or something.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.h:85: bool operator==(const
V8ObjectVarKey& other) const;
You don't need operator== for map or set. They rely on operator< exclusively...
!(a < b) && !(b < a) implies b == a.

Also, an optional suggestion/fyi: It's also fair game to only define a less-than
comparator for V8ObjectVar* and pass that when you instantiate a std::multiset,
rather than defining a separate Key type. There's a chance it will make the
usage simpler, but I haven't looked far enough to tell.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/host_var_tracker_unittest.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker_unittest.cc:52: virtual void
SetException(const char* message) OVERRIDE { DCHECK(false); }
NOTREACHED()?

[dmichael (off chromium), 2014-08-21 22:24:25]

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

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:147: 
Might be good to add a comment w/TODO...
// We don't forward this to the passthrough object; no plugins use
// that feature.
// TODO(raymes): Remove SetProperty support from PPP_Class.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:167:
internal_indexed_properties_.find(index);
The "SetReadonlyProperty" stuff is only used by the NaCl integration, and it
only uses string keys. You can get rid of internal_indexed_properties_ and
DCHECK or NOTREACHED if the key is not a string in SetReadOnlyProperty.

(It would probably make sense to change that API to use std::string, but Justin
or I can do that later; you don't have to mess with it here).

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:202: // throw an exception if
necessary.
We get here as a result of async IPC from the plugin; there's no place for the
exception to go. You can add a comment to that effect if you like, but there's
no need for a TODO.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:321: ScopedPPVar param =
try_catch.FromV8(message_data);
It feels kind of wrong that the "convert_objects" parameter is so far away from
the place where it's meaningful. I don't think it's obvious to a reader at all
what "true" means up there, and it doesn't make it obvious at this point that we
don't convert "Object" PP_Vars.

WDYT of moving the param to "FromV8" and "ToV8" instead of having it in the
constructor? The code might then look like...
// Messaging doesn't support "Object" PP_Vars.
const bool kConvertObjects = false;
ScopedPPVar param = try_catch.FromV8(message_data, kConvertObjects);

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:386: v8_var_converter.FromV8Value(
Looking at this code, it wasn't clear to me what told the V8VarConverter not to
make a Object PP_Var. You could use an enum and change it up to look like:
V8VarConverter v8_var_conterter(instance_->pp_instance(),
                                V8VarConverter::kDisallowObjectVars);
vs.
V8VarConverter v8_var_conterter(instance_->pp_instance(),
                                V8VarConverter::kAllowObjectVars);

The overloaded constructor, where one has a bool param and the other effectively
defaults the bool to false is kind of confusing and seems counter to usual
Google/Chromium style.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/message_channel.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.h:74: v8::Isolate* isolate) OVERRIDE;
Can you make these overrides for the Interceptors private please?

I prefer keeping them as restricted as possible to make the interface for
MessageChannel that is exposed to PepperPluginInstanceImpl clearer.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (left):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:627: // unregister
themselves inside the delete call).
If this comment is still true, I think we should keep it. If it's not true
anymore, then you don't need to swap.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:623: // Notify all the
plugin objects of deletion. This will prevent WebKit from
s/WebKit/blink ?

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:666: MessageChannel*
PepperPluginInstanceImpl::GetMessageChannel() {
Would it be simpler to have PepperPluginInstanceImpl just hold a
MessageChannel*? Or is holding the v8::Persistent not enough to guarantee the
MessageChannel stays alive?

If there's something more complicated that forces you in to doing it this way,
it's probably worth a comment.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:863:
MessageChannel::Create(this, &message_channel_);
Curious why this doesn't just return the handle? Did you maybe once-upon-a-time
have it returning the MessageChannel*?

I think having it return MessageChannel* and doing the handle out-param seems
reasonable.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2393: PepperTryCatchVar
try_catch(this, true, NULL);
optional nit: Stuff like "true" and NULL in callsites are a pet peeve of mine,
because I prefer if I don't have to go digging to understand the call. Maybe it
would suffice to throw in comments, e.g. "true /* convert_objects */"

(Alternatively, you could make an enum instead of bool and make the exception
param required... but I think even though I like it that way, it's probably
overkill to most devs)

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.h:146: // This pointer is
only valid on the stack.
The way I'm reading it, it makes a HandleScope that goes out of scope before
returning. So the pointer is only valid because of the Persistent handle, I
think. So the comment is misleading IMO.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_webplugin_impl.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_webplugin_impl.cc:150: return
v8::Local<v8::Object>();
nit: is that necessary, or would it be fine to just return result?

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/plugin_object.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/plugin_object.cc:98: return GetPropertyOrMethod(isolate,
PP_MakeInt32(index));
AFAIK PluginObject is only used by Flash and PDF. Looks like PDF only does
methods with string identifiers. Looks like Flash probably also only supports
string identifiers:
http://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/exte...

Maybe we can just drop the IndexedProperty stuff?

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/plugin_object.cc:225: argument_vars.Set(i,
try_catch.FromV8(arg).get());
Maybe it would make sense to have Set take a...
"const ScopedPPVar&"
...even though it stores a PP_Var internally. Then you wouldn't have to call
get() and the ref-counting details aren't exposed from ScopedPPVarArray. This
call as it is now looks slightly scary unless you look at the guts of Set to
understand that it will take a reference.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/plugin_object.cc:231: // object to ensure that it is not
destroyed courtsey an incoming
nit: courtsey>courtesy

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/plugin_object.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/plugin_object.h:60: v8::Isolate* isolate) OVERRIDE;
Please make the Interceptor stuff private.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/plugin_object.h:83: EnumerateProperties(v8::Isolate*
isolate);
I would prefer just out-params for the vectors.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/ppb_var_deprecated_impl.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/ppb_var_deprecated_impl.cc:158: *property_count = size;
nit: It kind of looks like the size out-param in Release isn't really necessary.
You could either call identifier_vars.size() release first, or use
identifier_vars->Length() here. The former seems like it ought to work anywhere.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/ppb_var_deprecated_impl.cc:206: ScopedPPVar
scoped_method(method_name);
nit: I think scoped_name might be clearer (scoped_method makes me think of an
object PP_Var representing an actual method)

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/ppb_var_deprecated_impl.cc:247:
accessor.instance()->container()->element().document().frame();
I think you have to check container() to be fully safe

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/ppb_var_deprecated_impl.cc:254:
function.As<v8::Function>(), recv, argc, converted_args.get());
I assume you checked that this matches existing behavior?

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/ppb_var_deprecated_impl.cc:280: return
PP_MakeUndefined();
Are you sure this is OK to eliminate?

https://codereview.chromium.org/459553003/diff/300001/ppapi/tests/test_instan...
File ppapi/tests/test_instance_deprecated.cc (right):

https://codereview.chromium.org/459553003/diff/300001/ppapi/tests/test_instan...
ppapi/tests/test_instance_deprecated.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
(I assume this and some other changes will disappear when your other CL lands)

[raymes, 2014-08-22 08:28:41]

Thanks for this!! I got busy with other reviews today and so I didn't get
through addressing all the comments but I will on Monday.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/host_var_tracker.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.cc:22: v8::Handle<v8::Object> object =
object_var->GetHandle();
On 2014/08/20 23:09:16, dmichael wrote:
> Could you use v8::Local here instead of Handle to make it clearer?

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.cc:91: V8ObjectVarKey(instance,
v8::Handle<v8::Object>()));
I thought about this a bit. It means you have to write something like:
std::pair<ObjectMap::const_iterator, ObjectMap::const_iterator> range =
object_map_.equal_range(V8ObjectVarKey(instance, v8::Handle<v8::Object>(),
V8ObjectVarKey(instance + 1, v8::Handle<v8::Object>());

And then technically you should handle overflow and it's still quite verbose. If
you feel strongly one way I'll be happy to change it :)

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/host_var_tracker.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.h:45: // Returns 0 if the instance
isn't known.
It's just used for tests so it's not important. I updated the name though to
reflect the fact that it's only for tests.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.h:85: bool operator==(const
V8ObjectVarKey& other) const;
On 2014/08/20 23:09:16, dmichael wrote:
> You don't need operator== for map or set. They rely on operator<
exclusively...
> !(a < b) && !(b < a) implies b == a.

Ahh I forgot that.

> 
> Also, an optional suggestion/fyi: It's also fair game to only define a
less-than
> comparator for V8ObjectVar* and pass that when you instantiate a
std::multiset,
> rather than defining a separate Key type. There's a chance it will make the
> usage simpler, but I haven't looked far enough to tell.

I tried doing something like this, but at least when I tried the code didn't
come out nicer. It meant that I had to construct a dummy V8ObjectVar to do some
of the lookups (e.g. in V8ObjectVarForV8Object)  and V8ObjectVars are tracked by
the var tracker, so it meant having to mess with that code. So I'd rather stick
with this :)

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/host_var_tracker_unittest.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker_unittest.cc:52: virtual void
SetException(const char* message) OVERRIDE { DCHECK(false); }
On 2014/08/20 23:09:16, dmichael wrote:
> NOTREACHED()?

Done.

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

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:147: 
On 2014/08/21 22:24:24, dmichael wrote:
> Might be good to add a comment w/TODO...
> // We don't forward this to the passthrough object; no plugins use
> // that feature.
> // TODO(raymes): Remove SetProperty support from PPP_Class.

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:167:
internal_indexed_properties_.find(index);
Thank you! Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:202: // throw an exception if
necessary.
Ah...I forgot about this. Done!

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:321: ScopedPPVar param =
try_catch.FromV8(message_data);
I think you're right, it is quite confusing. At the same time it makes the code
a bit messier at the conversion sites. How about this compromise:

-I removed the parameter altogether from PepperTryCatchVar because it was always
being set to true.
-I changed the constructor param to be the enum you suggested in V8VarConverter.

I understand your argument though, so let me know if you still prefer me to push
it out of the constructor :)

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:386: v8_var_converter.FromV8Value(
I agree, I like your suggestion I think it's much clearer. Thanks!

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/message_channel.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.h:74: v8::Isolate* isolate) OVERRIDE;
On 2014/08/21 22:24:24, dmichael wrote:
> Can you make these overrides for the Interceptors private please?
> 
> I prefer keeping them as restricted as possible to make the interface for
> MessageChannel that is exposed to PepperPluginInstanceImpl clearer.

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (left):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:627: // unregister
themselves inside the delete call).
On 2014/08/21 22:24:24, dmichael wrote:
> If this comment is still true, I think we should keep it. If it's not true
> anymore, then you don't need to swap.

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:623: // Notify all the
plugin objects of deletion. This will prevent WebKit from
On 2014/08/21 22:24:24, dmichael wrote:
> s/WebKit/blink ?

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:666: MessageChannel*
PepperPluginInstanceImpl::GetMessageChannel() {
Not completely sure what you mean. We need to store the v8::Persistent to ensure
the MessageChannel stays alive. We might be able to store a pointer to the
MessageChannel in addition to that but I feared that this was somehow unsafe
because V8 was managing the object. What do you think? Should I store the
pointer as well?

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:863:
MessageChannel::Create(this, &message_channel_);
Ok that sounds reasonable, I'll revisit it in the next patchset.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:2393: PepperTryCatchVar
try_catch(this, true, NULL);
On 2014/08/21 22:24:24, dmichael wrote:
> optional nit: Stuff like "true" and NULL in callsites are a pet peeve of mine,
> because I prefer if I don't have to go digging to understand the call. Maybe
it
> would suffice to throw in comments, e.g. "true /* convert_objects */"
> 
> (Alternatively, you could make an enum instead of bool and make the exception
> param required... but I think even though I like it that way, it's probably
> overkill to most devs)

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.h:146: // This pointer is
only valid on the stack.
On 2014/08/21 22:24:25, dmichael wrote:
> The way I'm reading it, it makes a HandleScope that goes out of scope before
> returning. So the pointer is only valid because of the Persistent handle, I
> think. So the comment is misleading IMO.

The reason is because the underlying v8 object could get destroyed at any point
in time, so you wouldn't want to store the pointer somewhere unless you knew
what you were doing. Does that make sense? Let me know if I can make the comment
more clear.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_webplugin_impl.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_webplugin_impl.cc:150: return
v8::Local<v8::Object>();
On 2014/08/21 22:24:25, dmichael wrote:
> nit: is that necessary, or would it be fine to just return result?

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/plugin_object.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/plugin_object.h:60: v8::Isolate* isolate) OVERRIDE;
These are called from MessageChannel

[dmichael (off chromium), 2014-08-22 16:28:47]

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/host_var_tracker.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.cc:91: V8ObjectVarKey(instance,
v8::Handle<v8::Object>()));
On 2014/08/22 08:28:39, raymes wrote:
> I thought about this a bit. It means you have to write something like:
> std::pair<ObjectMap::const_iterator, ObjectMap::const_iterator> range =
> object_map_.equal_range(V8ObjectVarKey(instance, v8::Handle<v8::Object>(),
> V8ObjectVarKey(instance + 1, v8::Handle<v8::Object>());
> 
> And then technically you should handle overflow and it's still quite verbose.
If
> you feel strongly one way I'll be happy to change it :)
> 
I don't see why or how you would pass two keys? I'm talking about
std::multimap::equal_range. I don't think there's any overflow to worry about...
when you get the two iterators back, you just iterate over that range (which
might be empty). I think it should be fairly clean.

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

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/message_channel.cc:321: ScopedPPVar param =
try_catch.FromV8(message_data);
On 2014/08/22 08:28:40, raymes wrote:
> I think you're right, it is quite confusing. At the same time it makes the
code
> a bit messier at the conversion sites. How about this compromise:
> 
> -I removed the parameter altogether from PepperTryCatchVar because it was
always
> being set to true.
> -I changed the constructor param to be the enum you suggested in
V8VarConverter.
> 
> I understand your argument though, so let me know if you still prefer me to
push
> it out of the constructor :)
This works for me, seems like a good compromise. It's pretty understandable now,
and moving it to the conversion calls would be unnecessarily verbose.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:666: MessageChannel*
PepperPluginInstanceImpl::GetMessageChannel() {
On 2014/08/22 08:28:40, raymes wrote:
> Not completely sure what you mean. We need to store the v8::Persistent to
ensure
> the MessageChannel stays alive. We might be able to store a pointer to the
> MessageChannel in addition to that but I feared that this was somehow unsafe
> because V8 was managing the object. What do you think? Should I store the
> pointer as well? 
That's all I meant, is I think it makes sense to *also* store the pointer. I
don't think it's any less safe, because the raw pointer you get from this
function is also only guaranteed to be pinned in memory while the Persistent is
alive.

https://codereview.chromium.org/459553003/diff/320001/content/renderer/pepper...
File content/renderer/pepper/pepper_try_catch.h (right):

https://codereview.chromium.org/459553003/diff/320001/content/renderer/pepper...
content/renderer/pepper/pepper_try_catch.h:38: // Whether To/FromV8 should
convert object vars. If set to false, an exception
nit: comment is out of date

[raymes, 2014-08-25 01:40:01]

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/host_var_tracker.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.cc:91: V8ObjectVarKey(instance,
v8::Handle<v8::Object>()));
Sorry it was late Friday afternoon and I was not thinking nor being careful :(

You're right. But in most of the cases (like this one) equal_range won't suffice
because we want to iterate through all the of the objects that have the same
instance, but not necessarily the same hash. That is why the key constructed
here has an empty handle, so that the lower bound will include all objects with
the given instance. Does that make sense?

There is one case (GetForV8Object) where equal_range works and it does simplify
it. I've used it there :)

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:666: MessageChannel*
PepperPluginInstanceImpl::GetMessageChannel() {
On 2014/08/22 16:28:46, dmichael wrote:
> On 2014/08/22 08:28:40, raymes wrote:
> > Not completely sure what you mean. We need to store the v8::Persistent to
> ensure
> > the MessageChannel stays alive. We might be able to store a pointer to the
> > MessageChannel in addition to that but I feared that this was somehow unsafe
> > because V8 was managing the object. What do you think? Should I store the
> > pointer as well? 
> That's all I meant, is I think it makes sense to *also* store the pointer. I
> don't think it's any less safe, because the raw pointer you get from this
> function is also only guaranteed to be pinned in memory while the Persistent
is
> alive.

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/plugin_object.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/plugin_object.cc:98: return GetPropertyOrMethod(isolate,
PP_MakeInt32(index));
I agree - I think it's highly unlikely it is used. I've stripped out this stuff
:)

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/plugin_object.cc:225: argument_vars.Set(i,
try_catch.FromV8(arg).get());
Good suggestion. I think I've done what you suggested.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/plugin_object.cc:231: // object to ensure that it is not
destroyed courtsey an incoming
On 2014/08/21 22:24:25, dmichael wrote:
> nit: courtsey>courtesy

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/plugin_object.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/plugin_object.h:83: EnumerateProperties(v8::Isolate*
isolate);
I've removed this based on the suggestion to eliminate the indexed property
stuff.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/ppb_var_deprecated_impl.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/ppb_var_deprecated_impl.cc:158: *property_count = size;
On 2014/08/21 22:24:25, dmichael wrote:
> nit: It kind of looks like the size out-param in Release isn't really
necessary.
> You could either call identifier_vars.size() release first, or use
> identifier_vars->Length() here. The former seems like it ought to work
anywhere.

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/ppb_var_deprecated_impl.cc:206: ScopedPPVar
scoped_method(method_name);
On 2014/08/21 22:24:25, dmichael wrote:
> nit: I think scoped_name might be clearer (scoped_method makes me think of an
> object PP_Var representing an actual method)

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/ppb_var_deprecated_impl.cc:247:
accessor.instance()->container()->element().document().frame();
On 2014/08/21 22:24:25, dmichael wrote:
> I think you have to check container() to be fully safe

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/ppb_var_deprecated_impl.cc:254:
function.As<v8::Function>(), recv, argc, converted_args.get());
The existing behavior was implemented in quite a different way and it called
deep into the guts of blink - I couldn't figure out if it matched exactly. But I
did ask jochen@ (see earlier in this review) and he said that this was the right
way to do it.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/ppb_var_deprecated_impl.cc:280: return
PP_MakeUndefined();
Yep - I verified with Xing last week: 
"[In VarPrivate] The following 2 methods are used:

AsScriptableObject()
Call()"

So there is actually more stuff I want to rip out but I'd rather do it as a
followup to this CL :) That's why I filed a bug last week.

https://codereview.chromium.org/459553003/diff/300001/ppapi/tests/test_instan...
File ppapi/tests/test_instance_deprecated.cc (right):

https://codereview.chromium.org/459553003/diff/300001/ppapi/tests/test_instan...
ppapi/tests/test_instance_deprecated.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Yes - sorry about including this :(

https://codereview.chromium.org/459553003/diff/320001/content/renderer/pepper...
File content/renderer/pepper/pepper_try_catch.h (right):

https://codereview.chromium.org/459553003/diff/320001/content/renderer/pepper...
content/renderer/pepper/pepper_try_catch.h:38: // Whether To/FromV8 should
convert object vars. If set to false, an exception
On 2014/08/22 16:28:47, dmichael wrote:
> nit: comment is out of date

Done.

[dmichael (off chromium), 2014-08-25 17:31:12]

lgtm, thanks!

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/host_var_tracker.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.cc:91: V8ObjectVarKey(instance,
v8::Handle<v8::Object>()));
On 2014/08/25 01:40:00, raymes wrote:
> Sorry it was late Friday afternoon and I was not thinking nor being careful :(
> 
> You're right. But in most of the cases (like this one) equal_range won't
suffice
> because we want to iterate through all the of the objects that have the same
> instance, but not necessarily the same hash. That is why the key constructed
> here has an empty handle, so that the lower bound will include all objects
with
> the given instance. Does that make sense?
> 
> There is one case (GetForV8Object) where equal_range works and it does
simplify
> it. I've used it there :)
Ah, I completely missed that detail. Maybe you can add a comment to call out the
empty handle trick you're doing?

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.h:146: // This pointer is
only valid on the stack.
On 2014/08/22 08:28:40, raymes wrote:
> On 2014/08/21 22:24:25, dmichael wrote:
> > The way I'm reading it, it makes a HandleScope that goes out of scope before
> > returning. So the pointer is only valid because of the Persistent handle, I
> > think. So the comment is misleading IMO.
> 
> The reason is because the underlying v8 object could get destroyed at any
point
> in time, so you wouldn't want to store the pointer somewhere unless you knew
> what you were doing. Does that make sense? Let me know if I can make the
comment
> more clear.
I guess my point is that the lifetime issue has nothing to do with whether the
pointer is on the stack. A comment is fine, but it would be better to say
something like...
"This pointer is only valid while the PepperPluginInstanceImpl is valid."
(AIUI, the v8 object can't get destroyed at any point... because
PepperPluginInstanceImpl pins it in memory via a Persistent handle)

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/plugin_object.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/plugin_object.h:60: v8::Isolate* isolate) OVERRIDE;
On 2014/08/22 08:28:40, raymes wrote:
> These are called from MessageChannel

Acknowledged.

[raymes, 2014-08-26 05:15:11]

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/host_var_tracker.cc (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/host_var_tracker.cc:91: V8ObjectVarKey(instance,
v8::Handle<v8::Object>()));
On 2014/08/25 17:31:11, dmichael wrote:
> On 2014/08/25 01:40:00, raymes wrote:
> > Sorry it was late Friday afternoon and I was not thinking nor being careful
:(
> > 
> > You're right. But in most of the cases (like this one) equal_range won't
> suffice
> > because we want to iterate through all the of the objects that have the same
> > instance, but not necessarily the same hash. That is why the key constructed
> > here has an empty handle, so that the lower bound will include all objects
> with
> > the given instance. Does that make sense?
> > 
> > There is one case (GetForV8Object) where equal_range works and it does
> simplify
> > it. I've used it there :)
> Ah, I completely missed that detail. Maybe you can add a comment to call out
the
> empty handle trick you're doing?

Done.

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.h (right):

https://codereview.chromium.org/459553003/diff/300001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.h:146: // This pointer is
only valid on the stack.
I agree. I removed that comment and I added a comment above the message_channel_
member in the last patch set :)

[raymes, 2014-08-28 00:43:45]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-08-28 00:45:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/459553003/500001

[commit-bot: I haz the power, 2014-08-28 02:12:29]

Committed patchset #26 (id:500001) as 21f446ae855d60cc896b40cb9a3249ed07f150b3

[raymes, 2014-08-28 04:49:43]

A revert of this CL (patchset #26) has been created in
https://codereview.chromium.org/512983004/ by raymes@chromium.org.

The reason for reverting is: Crash in telemetry tests
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_....

[vabr (Chromium), 2014-08-28 07:29:56]

On 2014/08/28 04:49:43, raymes wrote:
> A revert of this CL (patchset #26) has been created in
> https://codereview.chromium.org/512983004/ by mailto:raymes@chromium.org.
> 
> The reason for reverting is: Crash in telemetry tests
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_....

FYI, this CL also broke HostVarTrackerTest.ReuseVar on Linux Chromium OS ASan
LSan Tests (3) (and the revert fixed that). Please check that one as well before
re-landing.

Thanks for the quick revert!
Vaclav

[raymes, 2014-08-28 08:27:03]

Hey Dave,

PTAL at the diff for the last patch set if you get a chance. There were two
crashes this caused. One was due to the plugin instance being deleted prior to
the message channel. The other was due to the garbage collector not being run at
the right time in the HostVarTracker test. I've fixed both these and tried to
test for the UAF (unfortunately I ended up reusing AddPostCondition, aahh!)

Thanks!
Raymes

[dmichael (off chromium), 2014-08-28 15:50:35]

Honstly haven't looked carefully, but seems reasonable to me. Sad that we can't
be sure the Instance outlives the MessageChannel, but that seems almost
unavoidable. And...  I don't mind AddPostCondition being there if it's still
useful :-p

still lgtm

https://codereview.chromium.org/459553003/diff/520001/ppapi/tests/test_post_m...
File ppapi/tests/test_post_message.cc (right):

https://codereview.chromium.org/459553003/diff/520001/ppapi/tests/test_post_m...
ppapi/tests/test_post_message.cc:181:
instance_->AddPostCondition("window.pluginPostMessage('') === undefined");
nit: I think you ought to be able to do this all in one AddPostCondition? But
either way seems OK.

[raymes, 2014-08-29 01:33:41]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-08-29 01:34:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/459553003/520001

[commit-bot: I haz the power, 2014-08-29 02:46:07]

Committed patchset #27 (id:520001) as ee49e63baf57e503bd71dfe61c8a80df63eac9aa

[raymes, 2014-08-29 05:04:25]

A revert of this CL (patchset #27) has been created in
https://codereview.chromium.org/522583002/ by raymes@chromium.org.

The reason for reverting is: Failed memory tests:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....

[raymes, 2014-09-01 00:57:23]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-09-01 00:57:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/459553003/540001

[commit-bot: I haz the power, 2014-09-01 01:55:13]

Committed patchset #28 (id:540001) as e06bc5d896e494b9ec556f0e89dcc523778a1432

[raymes, 2014-09-03 05:47:32]

A revert of this CL (patchset #28 id:540001) has been created in
https://codereview.chromium.org/537483002/ by raymes@chromium.org.

The reason for reverting is: Causing a bunch of top crashes:
go/crash/c383c0c43c093ea1
go/crash/576a27ecf5f32094
go/crash/08fc74881d1819ac
go/crash/a6a00150ba55c0bf
.

[raymes, 2014-09-04 22:23:40]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2014-09-04 22:26:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/459553003/600001

[commit-bot: I haz the power, 2014-09-04 23:02:35]

Committed patchset #31 (id:600001) as 8d34dc7609c1e27fe4e6807b9cb2c9220e942fa2

[commit-bot: I haz the power, 2014-09-10 02:56:34]

Patchset 31 (id:??) landed as
https://crrev.com/6fe3f977401afbb8e62adf7912e9bd59c0445d02
Cr-Commit-Position: refs/heads/master@{#292295}

[commit-bot: I haz the power, 2014-09-10 03:05:43]

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

[commit-bot: I haz the power, 2014-09-10 03:15:16]

Patchset 31 (id:??) landed as
https://crrev.com/9111ba5a6248ce85a5f983e376823387055c3011
Cr-Commit-Position: refs/heads/master@{#292823}

[commit-bot: I haz the power, 2014-09-10 03:33:41]

Patchset 31 (id:??) landed as
https://crrev.com/2515b7dc2832404db4aa8d7d20257f0865c0d931
Cr-Commit-Position: refs/heads/master@{#293366}