Don't send PP_Vars/V8 values with cycles across PostMessage

Don't send PP_Vars/V8 values with cycles across PostMessage

This prevents PP_Vars/V8 values with cycles being transmitted across PostMessage. An undefined value will be sent instead and an error will be logged to the console.

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

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

[raymes, 2013-06-03 20:08:51]

How did this become more complex than allowing cycles!

A few notes:
-I don't think that the conversion code can really be simplified by not allowing
cycles because we still support multiple references to the same vars so I have
just added a boolean paramater to the conversion classes.
-It would have been nice to only do this detection in message_channel.cc. The
problem is that when we send a var with cycles to the browser, we will leak it
unless we have our Destroy() function (which we haven't written yet). I realized
we were actually leaking vars in our current implementation.
-So right now I've added the detection/logging in serialized_var.cc. This isn't
ideal. Once we have a Destroy() written, we can do the detection only in
message_channel and get rid of this.

[dmichael (off chromium), 2013-06-04 16:59:10]

https://codereview.chromium.org/16140011/diff/5002/ppapi/api/ppb_messaging.idl
File ppapi/api/ppb_messaging.idl (right):

https://codereview.chromium.org/16140011/diff/5002/ppapi/api/ppb_messaging.id...
ppapi/api/ppb_messaging.idl:40: * an undefined var will be sent instead and an
error will be logged to the
Is this the behavior that was discussed in the meeting last week? I would have
assumed we would send no message to the other side (But log an error).

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

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/raw_var_data.c...
ppapi/proxy/raw_var_data.cc:42: // Used to track parent nodes on the stack as
traversing the graph.
nit: as->while

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/raw_var_data.c...
ppapi/proxy/raw_var_data.cc:97: if
(::ppapi::VarTracker::IsVarTypeRefcounted(var.type))
I don't understand why you're looking at the type of |var| here. Did you mean
|current_var|?

(If |var| is not ref-counted, we could avoid pretty much the whole function... 
it doesn't make sense to me to check its type inside the loop.)

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/raw_var_data.h
File ppapi/proxy/raw_var_data.h (right):

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/raw_var_data.h...
ppapi/proxy/raw_var_data.h:55: bool allow_cycles);
Why have this parameter? Why not just disallow cycles entirely? Do we ever pass
true?

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/serialized_var.cc
File ppapi/proxy/serialized_var.cc (right):

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/serialized_var...
ppapi/proxy/serialized_var.cc:127: kSerializationError +
ppapi::Var::PPVarToLogString(var_));
Yeah...  this is going to result in sending a message while inside the
serialization for another message. It probably works fine, but it seems like
something we ought to try to avoid.

I wonder if we could make an invalid form of a serialized Var, so that the
recipient will receive the invalid Var, know something went wrong, and can take
appropriate action there (log and don't send a message, in the case of
Messaging). [This would be purely an implementation detail; we wouldn't expose
the invalid kind of var to plugins]

https://codereview.chromium.org/16140011/diff/5002/ppapi/tests/test_post_mess...
File ppapi/tests/test_post_message.cc (right):

https://codereview.chromium.org/16140011/diff/5002/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:244: "    plugin.postMessage(";
nit: inner code should line up with "var", not "plugin", right?

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/messa...
File webkit/plugins/ppapi/message_channel.cc (right):

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:57: return kV8ToVarConversionError +
std::string(*utf8, utf8.length());
Are you actually converting the v8_value to a string? That can probably be
pretty big, can't it? I think I'd rather just not give that level of detail.
WDYT?

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:489: message_data = PP_MakeUndefined();
This feels kind of klugy to me. My first choice is to not send "Undefined" in
the case of error. My second choice, if you disagree with that, would be to do
the logging outside of this function, and pass this function an Undefined
instead of doing this. I think it's easier to understand if its only
responsibility is sending |message_data|.

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/v8_va...
File webkit/plugins/ppapi/v8_var_converter.h (right):

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/v8_va...
webkit/plugins/ppapi/v8_var_converter.h:21: V8VarConverter(bool allow_cycles);
Better to just always disallow them, I think. We can dig the cycle passing stuff
out of source control; no need to maintain a dead branch of code.

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/v8_va...
webkit/plugins/ppapi/v8_var_converter.h:26: v8::Handle<v8::Value>* result)
const;
side issue: Looking at this now, these should probably be static, and the
default constructor could be private & unimplemented (or omitted; I think
declaring a copy ctor in DISALLOW_COPY_AND_ASSIGN might be enough for the
compiler to not generate a default constructor). 

(V8VarConverter could even be a namespace instead of a utility class).

[raymes, 2013-06-04 19:36:06]

Thanks!

https://codereview.chromium.org/16140011/diff/5002/ppapi/api/ppb_messaging.idl
File ppapi/api/ppb_messaging.idl (right):

https://codereview.chromium.org/16140011/diff/5002/ppapi/api/ppb_messaging.id...
ppapi/api/ppb_messaging.idl:40: * an undefined var will be sent instead and an
error will be logged to the
We discussed both and there wasn't a clear decision. I think both make sense (in
one way it's nice for an application to get notified if the var is ignored). But
I don't mind either way. I changed it to drop the var.

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

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/raw_var_data.c...
ppapi/proxy/raw_var_data.cc:42: // Used to track parent nodes on the stack as
traversing the graph.
On 2013/06/04 16:59:10, dmichael wrote:
> nit: as->while

Done.

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/raw_var_data.c...
ppapi/proxy/raw_var_data.cc:97: if
(::ppapi::VarTracker::IsVarTypeRefcounted(var.type))
Yes...good catch. Thanks

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/raw_var_data.h
File ppapi/proxy/raw_var_data.h (right):

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/raw_var_data.h...
ppapi/proxy/raw_var_data.h:55: bool allow_cycles);
On 2013/06/04 16:59:10, dmichael wrote:
> Why have this parameter? Why not just disallow cycles entirely? Do we ever
pass
> true?

Done.

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/serialized_var.cc
File ppapi/proxy/serialized_var.cc (right):

https://codereview.chromium.org/16140011/diff/5002/ppapi/proxy/serialized_var...
ppapi/proxy/serialized_var.cc:127: kSerializationError +
ppapi::Var::PPVarToLogString(var_));
I actually went down this road first and then backtracked on it because it felt
a bit nasty as well :P But I do prefer it. I've changed it to do that.

https://codereview.chromium.org/16140011/diff/5002/ppapi/tests/test_post_mess...
File ppapi/tests/test_post_message.cc (right):

https://codereview.chromium.org/16140011/diff/5002/ppapi/tests/test_post_mess...
ppapi/tests/test_post_message.cc:244: "    plugin.postMessage(";
On 2013/06/04 16:59:10, dmichael wrote:
> nit: inner code should line up with "var", not "plugin", right?

Done.

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/messa...
File webkit/plugins/ppapi/message_channel.cc (right):

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:57: return kV8ToVarConversionError +
std::string(*utf8, utf8.length());
That's ok with me.

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/messa...
webkit/plugins/ppapi/message_channel.cc:489: message_data = PP_MakeUndefined();
This was kludgy. Hopefully my change seems nicer.

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/v8_va...
File webkit/plugins/ppapi/v8_var_converter.h (right):

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/v8_va...
webkit/plugins/ppapi/v8_var_converter.h:21: V8VarConverter(bool allow_cycles);
On 2013/06/04 16:59:10, dmichael wrote:
> Better to just always disallow them, I think. We can dig the cycle passing
stuff
> out of source control; no need to maintain a dead branch of code.

Done.

https://codereview.chromium.org/16140011/diff/5002/webkit/plugins/ppapi/v8_va...
webkit/plugins/ppapi/v8_var_converter.h:26: v8::Handle<v8::Value>* result)
const;
On 2013/06/04 16:59:10, dmichael wrote:
> side issue: Looking at this now, these should probably be static, and the
> default constructor could be private & unimplemented (or omitted; I think
> declaring a copy ctor in DISALLOW_COPY_AND_ASSIGN might be enough for the
> compiler to not generate a default constructor). 
> 
> (V8VarConverter could even be a namespace instead of a utility class).

Done.

[dmichael (off chromium), 2013-06-05 17:05:47]

Looks pretty good overall. I just have some suggestions (possibly bad) for
simplifying the graph copy.

https://codereview.chromium.org/16140011/diff/5002/ppapi/api/ppb_messaging.idl
File ppapi/api/ppb_messaging.idl (right):

https://codereview.chromium.org/16140011/diff/5002/ppapi/api/ppb_messaging.id...
ppapi/api/ppb_messaging.idl:40: * an undefined var will be sent instead and an
error will be logged to the
On 2013/06/04 19:36:06, raymes wrote:
> We discussed both and there wasn't a clear decision. I think both make sense
(in
> one way it's nice for an application to get notified if the var is ignored).
But
> I don't mind either way. I changed it to drop the var.
Yeah, I could see both sides. Dropping it seems less surprising to me, I guess.
I think passing a message with unexpected contents might be more likely to
result in weird/bad behavior to a user.

Hopefully the log message provides enough notification to the developer and/or
user.

https://codereview.chromium.org/16140011/diff/21001/ppapi/api/ppb_messaging.idl
File ppapi/api/ppb_messaging.idl (right):

https://codereview.chromium.org/16140011/diff/21001/ppapi/api/ppb_messaging.i...
ppapi/api/ppb_messaging.idl:40: * the var will be dropped and an error will be
logged to the console.
Might as well be crystal clear, "var will be dropped" -> "message will not be
sent". (Ditto for PPP)

We should also update the C++ documentation.

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

https://codereview.chromium.org/16140011/diff/21001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:80: scoped_ptr<RawVarDataGraph> graph(new
RawVarDataGraph);
Looking at this algorithm again, it seems like it could be simplified a little
bit. Might lead to a little less code and a little bit better performance (same
complexity, just less revisiting nodes and/or pushing/popping stuff on the
stack)

I sprinkled some comments in below that go together (i.e., some might not make
complete sense without the other suggestions).

Tangentially, you might be able to make the algorithm a little easier to read
and reason about by making the stack slightly more sophisticated:
struct StackFrame {
  size_t parent_index;
  queue<size_t> children;  // stack or vector are fine, too
};
(I think you only need to know index, but you could add PP_Var if it helps with
something).

The idea is you look at the current stack top's children. If it's non-empty, pop
a child from the queue, create a new StackFrame for it, and push it on the
stack. If the queue of children *is* empty, you know you're done with that
StackFrame and can pop it.

I don't know if this will actually be more readable, but it just separates the
"sentinel" values from the non-sentinel ones more explicitly. Feel free to try
it or ignore it.

In either case, a short description of the algorithm somewhere would be helpful.

https://codereview.chromium.org/16140011/diff/21001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:94: if (stack.top().sentinel) {
Can you avoid adding the new "sentinel" thing by just checking whether the item
has been initialized? (This goes along with the comment on 134 below...)

If it's been initialized, its children have already been added, and we can pop
it.

https://codereview.chromium.org/16140011/diff/21001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:97: parent_ids.erase(current_var.value.as_id);
Here, if you only put in types that can have children, you don't need to check
if it's ref-counted. Just erase it.

https://codereview.chromium.org/16140011/diff/21001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:128: parent_ids.count(child.value.as_id) != 0) {
How about an explicit comment here (and below) that this is to detect and
prevent cycles

https://codereview.chromium.org/16140011/diff/21001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:134: stack.push(StackEntry(child, child_id));
How about checking at this point whether or not:
1) The type can have children. If not, no need to push it on the stack.
2) Whether the type is initialized. If so, no need to push it on the stack. It's
already been processed.

https://codereview.chromium.org/16140011/diff/21001/ppapi/tests/test_post_mes...
File ppapi/tests/test_post_message.cc (right):

https://codereview.chromium.org/16140011/diff/21001/ppapi/tests/test_post_mes...
ppapi/tests/test_post_message.cc:598: ASSERT_EQ(WaitForMessages(), 1);
Shouldn't we expect 0 here now?

https://codereview.chromium.org/16140011/diff/21001/webkit/plugins/ppapi/v8_v...
File webkit/plugins/ppapi/v8_var_converter.cc (right):

https://codereview.chromium.org/16140011/diff/21001/webkit/plugins/ppapi/v8_v...
webkit/plugins/ppapi/v8_var_converter.cc:233: v8::Handle<v8::Value>* result) {
Without looking carefully, it might be possible to simplify this a bit in the
same ways as RawVarDataGraph

[raymes, 2013-06-05 22:48:26]

Thanks!

https://codereview.chromium.org/16140011/diff/21001/ppapi/api/ppb_messaging.idl
File ppapi/api/ppb_messaging.idl (right):

https://codereview.chromium.org/16140011/diff/21001/ppapi/api/ppb_messaging.i...
ppapi/api/ppb_messaging.idl:40: * the var will be dropped and an error will be
logged to the console.
On 2013/06/05 17:05:47, dmichael wrote:
> Might as well be crystal clear, "var will be dropped" -> "message will not be
> sent". (Ditto for PPP)
> 
> We should also update the C++ documentation.

Done.

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

https://codereview.chromium.org/16140011/diff/21001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:80: scoped_ptr<RawVarDataGraph> graph(new
RawVarDataGraph);
I agree that it's not the easiest to understand. Those changes might improve it
a little, I think it's hard to say without trying it.

I think the biggest way to improve this (in terms of making it easy to
understand/reason about) would be to use the visitor pattern to implement the
algorithm. Basically to pull out the traversal/cycle-detection logic into one
class and the conversion logic into another class. Then we could re-use the
traversal logic for the 3 cases we currently have where we traverse the var
graph and only implement the conversion logic there.

Of course this is at /slight/ odds with making the thing really efficient. For
example, right now we are able to add the children to the ArrayRawVarData during
the traversal (i.e. right before we add them to the stack). If we separated that
logic then we would end up traversing the children during the conversion as
well. I think the tradeoff is worth it though.

I toyed with this idea before I started to write this stuff but thought it might
be overdesigning it a bit. Also, it only helps us in the Var-to-something else
case (e.g. it doesn't help us when going from v8 value back to var unless we
write a visitor for v8 values). But then again there's a bunch of places where
we traverse v8 values and base values so maybe it's just a pattern we should use
more often :)

https://codereview.chromium.org/16140011/diff/21001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:94: if (stack.top().sentinel) {
Yes, good catch.

https://codereview.chromium.org/16140011/diff/21001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:97: parent_ids.erase(current_var.value.as_id);
But then the base case changes (i.e. you have to make sure the starting node
doesn't have children and initialize it properly which duplicates part of the
body of this loop). I think it's a tradeoff.

https://codereview.chromium.org/16140011/diff/21001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:128: parent_ids.count(child.value.as_id) != 0) {
On 2013/06/05 17:05:47, dmichael wrote:
> How about an explicit comment here (and below) that this is to detect and
> prevent cycles

Done.

https://codereview.chromium.org/16140011/diff/21001/ppapi/proxy/raw_var_data....
ppapi/proxy/raw_var_data.cc:134: stack.push(StackEntry(child, child_id));
Even if the node doesn't have children, it has to have Init() called on it. So I
could pull that logic down here, but then the sentinel logic changes :P

https://codereview.chromium.org/16140011/diff/21001/ppapi/tests/test_post_mes...
File ppapi/tests/test_post_message.cc (right):

https://codereview.chromium.org/16140011/diff/21001/ppapi/tests/test_post_mes...
ppapi/tests/test_post_message.cc:598: ASSERT_EQ(WaitForMessages(), 1);
Yeah - I thought that WaitForMessages() wouldn't work if we weren't expecting a
message so I posted an undefined var right after, but it seems to work fine.

[dmichael (off chromium), 2013-06-06 16:32:30]

lgtm, thanks!

I thought about visitor pattern too. With 3 places where we've implemented this
algorithm, it *might* be worth it :-/. I've never cared for that pattern very
much; all the indirection often makes things less understandable IMHO.

https://codereview.chromium.org/16140011/diff/21001/ppapi/tests/test_post_mes...
File ppapi/tests/test_post_message.cc (right):

https://codereview.chromium.org/16140011/diff/21001/ppapi/tests/test_post_mes...
ppapi/tests/test_post_message.cc:598: ASSERT_EQ(WaitForMessages(), 1);
On 2013/06/05 22:48:26, raymes wrote:
> Yeah - I thought that WaitForMessages() wouldn't work if we weren't expecting
a
> message so I posted an undefined var right after, but it seems to work fine.

It sends its own special message (that's echoed back) that goes in the queue
behind whatever messages you're really waiting on, and quits the message loop
when that one arrives. That's how it knows how many messages you got.

[raymes, 2013-06-18 00:11:19]

David, please take one more look at the delta between patchset 10 and 11. I had
to change a couple of things to fix the bug I mentioned.
Thanks!

[dmichael (off chromium), 2013-06-18 16:27:02]

Can you elaborate on the bug?

I have a couple of comments, but still lgtm overall.

https://codereview.chromium.org/16140011/diff/68001/ppapi/proxy/serialized_var.h
File ppapi/proxy/serialized_var.h (right):

https://codereview.chromium.org/16140011/diff/68001/ppapi/proxy/serialized_va...
ppapi/proxy/serialized_var.h:87: const HandleWriter& handle_writer) const {
Style nit: output params should come after input params
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...)

https://codereview.chromium.org/16140011/diff/68001/webkit/plugins/ppapi/v8_v...
File webkit/plugins/ppapi/v8_var_converter_unittest.cc (right):

https://codereview.chromium.org/16140011/diff/68001/webkit/plugins/ppapi/v8_v...
webkit/plugins/ppapi/v8_var_converter_unittest.cc:155: V8VarConverter converter;
It's weird that you create this before setting up the handle scope and context
scope. I guess it's because it actually has no state that it's okay. I think it
would be better to put after the scopes.

(optional suggestion: Maybe V8VarConverter should just be a namespace)

https://codereview.chromium.org/16140011/diff/68001/webkit/plugins/ppapi/v8_v...
webkit/plugins/ppapi/v8_var_converter_unittest.cc:159:
v8::Local<v8::Context>::New(isolate_, context_);
I don't have a very strong grasp on the v8 embedding API. Why do you need a new
Context?

[raymes, 2013-06-18 16:40:20]

Thanks!

> Can you elaborate on the bug?

The bug was because I introduced the notion of an invalid SerializedVar. I added
a bool at the start of serializing the var indicating whether it was valid. The
problem was when rewriting the handles, this wasn't happening because I was
pulling out the RawVarData and writing it straight to the message. So instead, I
made it go through SerializedVar to ensure the validity bool gets written.

https://codereview.chromium.org/16140011/diff/68001/ppapi/proxy/serialized_var.h
File ppapi/proxy/serialized_var.h (right):

https://codereview.chromium.org/16140011/diff/68001/ppapi/proxy/serialized_va...
ppapi/proxy/serialized_var.h:87: const HandleWriter& handle_writer) const {
Ah ok. I'll make this change in a followup CL if that's ok because if I change
it here, really I should change the order in all of RawVarData.

https://codereview.chromium.org/16140011/diff/68001/webkit/plugins/ppapi/v8_v...
File webkit/plugins/ppapi/v8_var_converter_unittest.cc (right):

https://codereview.chromium.org/16140011/diff/68001/webkit/plugins/ppapi/v8_v...
webkit/plugins/ppapi/v8_var_converter_unittest.cc:155: V8VarConverter converter;
This was a merge bug which I fixed in the latest patch. V8VarConverter is a
namespace now.

https://codereview.chromium.org/16140011/diff/68001/webkit/plugins/ppapi/v8_v...
webkit/plugins/ppapi/v8_var_converter_unittest.cc:159:
v8::Local<v8::Context>::New(isolate_, context_);
marja@ updated this in https://chromiumcodereview.appspot.com/16690003. I
haven't looked into the details but I assume she knows what she's doing!

[commit-bot: I haz the power, 2013-06-18 16:42:02]

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

[dmichael (off chromium), 2013-06-18 16:51:34]

(still lgtm)

https://codereview.chromium.org/16140011/diff/68001/ppapi/proxy/serialized_var.h
File ppapi/proxy/serialized_var.h (right):

https://codereview.chromium.org/16140011/diff/68001/ppapi/proxy/serialized_va...
ppapi/proxy/serialized_var.h:87: const HandleWriter& handle_writer) const {
On 2013/06/18 16:40:20, raymes wrote:
> Ah ok. I'll make this change in a followup CL if that's ok because if I change
> it here, really I should change the order in all of RawVarData.
Okay, sorry I didn't catch that sooner, then.

https://codereview.chromium.org/16140011/diff/68001/webkit/plugins/ppapi/v8_v...
File webkit/plugins/ppapi/v8_var_converter_unittest.cc (right):

https://codereview.chromium.org/16140011/diff/68001/webkit/plugins/ppapi/v8_v...
webkit/plugins/ppapi/v8_var_converter_unittest.cc:159:
v8::Local<v8::Context>::New(isolate_, context_);
On 2013/06/18 16:40:20, raymes wrote:
> marja@ updated this in https://chromiumcodereview.appspot.com/16690003. I
> haven't looked into the details but I assume she knows what she's doing!

Nevermind, I was reading it incorrectly. |context| is just a local handle to
|context_|, not a _new_ context. It might be unnecessary (?) but it's certainly
not wrong.

[commit-bot: I haz the power, 2013-06-18 18:43:04]

Change committed as 207040

[commit-bot: I haz the power, 2013-06-18 20:24:32]

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

[commit-bot: I haz the power, 2013-06-19 03:10:37]

Change committed as 207145