Browser Plugin: Fix Events

Browser Plugin: Fix Events

This is a set of short-term fixes for BrowserPlugin events to address abarth@'s concerns.

1. Use frame's v8 context instead of creating a new one.
2. Use v8::Local<v8::Object> type for local handle to event objects.
3. Copy listener event listener vector before executing listeners to avoid referencing member variables in case one of the listeners deallocates the BrowserPlugin.

A longer term fix will involve exposing CustomEvent to the WebKit API and constructing
WebCustomEvents in browser_plugin.cc.

BUG=155044
TEST=BrowserPluginHostTest.*, BrowserPluginTest.* pass

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161123

[abarth-chromium, 2012-10-09 18:38:17]

This is a big improvement.  Below are some comments we might want to address
before landing.

We might not be able to address the |frame| being destroyed issue in this patch,
but we should clean up the usage of the V8 API.

http://codereview.chromium.org/11086025/diff/1/content/renderer/browser_plugi...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/11086025/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:335:
frame->callFunctionEvenIfScriptDisabled(*it,
How do you know that this call doesn't destroy |frame| ?

http://codereview.chromium.org/11086025/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:353:
v8::Local<v8::Value>::New(v8::String::New(src_.c_str()));
It's more efficient to the two-argument form of v8::String::New.

Also, there's no reason to create a new local handle.  v8::String::New already
returns a local handle.

http://codereview.chromium.org/11086025/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:375:
v8::Local<v8::Object>::New(v8::Object::New());
v8::Object::New already returns a local handle.  There's no reason to create
another one.

http://codereview.chromium.org/11086025/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:377:
v8::String::New(url.spec().c_str(), url.spec().size()));
There's no reason to call c_str here because you don't need the string to be
null terminated.  Instead, you can call data(), which might avoid an allocation.

[Fady Samuel, 2012-10-09 20:25:11]

http://codereview.chromium.org/11086025/diff/1/content/renderer/browser_plugi...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/11086025/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:335:
frame->callFunctionEvenIfScriptDisabled(*it,
On 2012/10/09 18:38:18, abarth wrote:
> How do you know that this call doesn't destroy |frame| ?

Done.

http://codereview.chromium.org/11086025/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:353:
v8::Local<v8::Value>::New(v8::String::New(src_.c_str()));
On 2012/10/09 18:38:18, abarth wrote:
> It's more efficient to the two-argument form of v8::String::New.
> 
> Also, there's no reason to create a new local handle.  v8::String::New already
> returns a local handle.

Done.

http://codereview.chromium.org/11086025/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:375:
v8::Local<v8::Object>::New(v8::Object::New());
On 2012/10/09 18:38:18, abarth wrote:
> v8::Object::New already returns a local handle.  There's no reason to create
> another one.

Done.

http://codereview.chromium.org/11086025/diff/1/content/renderer/browser_plugi...
content/renderer/browser_plugin/browser_plugin.cc:377:
v8::String::New(url.spec().c_str(), url.spec().size()));
On 2012/10/09 18:38:18, abarth wrote:
> There's no reason to call c_str here because you don't need the string to be
> null terminated.  Instead, you can call data(), which might avoid an
allocation.

Done.

[abarth-chromium, 2012-10-09 20:30:00]

LGTM.  This patch is a vast improvement.  Thank you so much.

http://codereview.chromium.org/11086025/diff/5001/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/11086025/diff/5001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:335: if
(plugin.document().frame()) {
I would have put this in a local variable, but I'm a paranoid panda.  :)

WebFrame* frame = plugin.document().frame();
if (frame)
  frame->callFunctionEvenIfScriptDisabled(...)

http://codereview.chromium.org/11086025/diff/5001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:438:
v8::Local<v8::Object>::New(v8::Object::New());
No need to create a new local handle here either.  :)

http://codereview.chromium.org/11086025/diff/5001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:440:
v8::String::New(old_url.spec().c_str(), old_url.spec().size()));
c_str -> data

http://codereview.chromium.org/11086025/diff/5001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:451: // Fire the event
listener.
I would remove this comment since it doesn't really add anything to what the
code already said, but that might be WebKit style leaking into my review.

[Fady Samuel, 2012-10-09 21:35:21]

http://codereview.chromium.org/11086025/diff/5001/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/11086025/diff/5001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:335: if
(plugin.document().frame()) {
On 2012/10/09 20:30:00, abarth wrote:
> I would have put this in a local variable, but I'm a paranoid panda.  :)
> 
> WebFrame* frame = plugin.document().frame();
> if (frame)
>   frame->callFunctionEvenIfScriptDisabled(...)

Done.

http://codereview.chromium.org/11086025/diff/5001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:438:
v8::Local<v8::Object>::New(v8::Object::New());
On 2012/10/09 20:30:00, abarth wrote:
> No need to create a new local handle here either.  :)

Done.

http://codereview.chromium.org/11086025/diff/5001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:440:
v8::String::New(old_url.spec().c_str(), old_url.spec().size()));
On 2012/10/09 20:30:00, abarth wrote:
> c_str -> data

Done.

http://codereview.chromium.org/11086025/diff/5001/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:451: // Fire the event
listener.
On 2012/10/09 20:30:00, abarth wrote:
> I would remove this comment since it doesn't really add anything to what the
> code already said, but that might be WebKit style leaking into my review.

Done.

[Fady Samuel, 2012-10-09 22:45:25]

Hi Charlie,

Could you please take a look at this as OWNER?

Thanks,

Fady

[Charlie Reis, 2012-10-10 00:05:09]

Great, I'm glad we're improving this.  Thanks for your help, Adam!

Just a few comments.

http://codereview.chromium.org/11086025/diff/2003/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/11086025/diff/2003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:327: WebKit::WebElement plugin
= container()->element();
nit: It looks like you never use plugin directly, just
plugin.document().frame().  Why not grab it up front?  And is a null check
needed here as well?

http://codereview.chromium.org/11086025/diff/2003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:336: if (frame)
If this null check is to guard against frame being deleted, then it won't help,
will it?  The pointer doesn't get nulled when frame is deleted; it's just
garbage or some other object the next time around.

As Adam mentioned, it's probably a larger design issue to ensure the plugin
isn't deleted by one of these listeners.  A TODO with a new bug number would
help us keep track of that.

[abarth-chromium, 2012-10-10 00:21:40]

> nit: It looks like you never use plugin directly, just
> plugin.document().frame().  Why not grab it up front?

It is not possible to hold on to a WebFrame through the API.  The lifetime of
the WebFrame is managed by WebCore.  You can hold onto DOM nodes, however, which
is what this code does not.

> And is a null check needed here as well?

Probably not.  If the plugin exists, the element will be connected to the frame.
 It's only after the plugin dies that the element is disconnected from the
frame.

> If this null check is to guard against frame being deleted, then it won't
help,
> will it?

Why not?

> The pointer doesn't get nulled when frame is deleted; it's just
> garbage or some other object the next time around.

That's why we hold on to the node.  The Document's pointer to the Frame does get
nulled out.

> As Adam mentioned, it's probably a larger design issue to ensure the plugin
> isn't deleted by one of these listeners.  A TODO with a new bug number would
> help us keep track of that.

Yes.  There are still bugs in this patch in that case.  For example, although
you've made a copy of the listener array, you haven't actually retained the V8
objects in the array.  If the plugin get torn down, you'll Dispose those handles
and you'll be using garbage references to v8 objects.

This patch is a big improvement, but there are still tricky design issues to
address.

[Fady Samuel, 2012-10-10 00:25:12]

> Yes.  There are still bugs in this patch in that case.  For example, although
> you've made a copy of the listener array, you haven't actually retained the V8
> objects in the array.  If the plugin get torn down, you'll Dispose those
handles
> and you'll be using garbage references to v8 objects.
> 
> This patch is a big improvement, but there are still tricky design issues to
> address.

Hmm, when we make a copy of the array are we not also calling copy constructors
to the elements within the array? These elements are persistent handles, so
aren't we creating new persistent handles and so the lifetime of the v8 objects
in the array is equal to or greater than the lifetime of the array copy itself?

[abarth-chromium, 2012-10-10 00:32:02]

> Hmm, when we make a copy of the array are we not also calling copy
constructors
> to the elements within the array?

You are.

> These elements are persistent handles, so
> aren't we creating new persistent handles and so the lifetime of the v8
objects
> in the array is equal to or greater than the lifetime of the array copy
itself?

That's not how v8::Persistent works.  It's easier to think about v8::Persistent
like a raw pointer to an object in C++.  You need to call New and Dispose
exactly the same number of times, just like you call new and delete in C++
exactly the same number of times.  The copy constructor doesn't call New or
Dispose, just like the copy constructor for a raw C++ pointer doesn't call new
or delete.

In WebCore, we have classes like
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/ScopedPersist...
to help us do this correctly, just like we have scoped_ptr for C++ pointers. 
Here you're having to re-invent the wheel.

[Charlie Reis, 2012-10-10 02:46:13]

http://codereview.chromium.org/11086025/diff/2003/content/renderer/browser_pl...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/11086025/diff/2003/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:336: if (frame)
Ok, I missed the fact that document()'s reference to the Frame would be nulled
out.

So, LGTM if you add a TODO for the plugin deletion issue.

[commit-bot: I haz the power, 2012-10-10 14:24:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11086025/12002

[commit-bot: I haz the power, 2012-10-10 16:24:36]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-10-10 16:33:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11086025/12002

[commit-bot: I haz the power, 2012-10-10 16:33:49]

Change committed as 161123