browser-plugin: Remove event handling code, and use CustomEvents in webkit instead.

browser-plugin: Remove event handling code, and use CustomEvents in webkit instead.

The browser-plugin emits signals prefixed with '-internal-'. The webview shim
receives the events, and emits appropriately named polyfilled events. The webview
consumers should listen for these events, instead of the '-internal-' events.

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

[abarth-chromium, 2012-11-05 19:07:14]

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

http://codereview.chromium.org/11368071/diff/2006/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:320:
plugin.document().frame()->mainWorldScriptContext();
Why is the main world the correct world?

http://codereview.chromium.org/11368071/diff/2006/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:325: "});",
event_name.c_str());
Where does event_name come from?  Could this lead to XSS?

http://codereview.chromium.org/11368071/diff/2006/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:329: v8::Local<v8::Object>
event = value->ToObject();
Why not use the WebKit API to create a CustomEvent?  It seems like a hack to use
eval to do it.

http://codereview.chromium.org/11368071/diff/2006/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:337: v8::ReadOnly);
This operation can trigger JavaScript setters, which can do arbitrary things
like delete the BrowserPlugin.

http://codereview.chromium.org/11368071/diff/2006/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:463:
TriggerEvent(kExitEventName, &props);
It looks like you're using constants.  Can we pass the name of the event as an
enum value and then have the callee look up the string so that we know it is
always something that won't cause XSS?

[sadrul, 2012-11-06 03:23:34]

Addressed all the comments.

Note that because of this change, the event handlers now have to access the
custom data from 'event.detail' (instead of directly from 'event'). This has the
side-effect of an event-handler being able to modify the data in event.detail
(which may affect the subsequent event-handlers in the same dispatch cycle). I
have updated the tests accordingly.

The webkit patch this depends on is here:
https://bugs.webkit.org/show_bug.cgi?id=101293

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

http://codereview.chromium.org/11368071/diff/2006/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:320:
plugin.document().frame()->mainWorldScriptContext();
On 2012/11/05 19:07:15, abarth wrote:
> Why is the main world the correct world?

The current code uses the main-world context, so I am using the same here. But
should this be different?

http://codereview.chromium.org/11368071/diff/2006/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:325: "});",
event_name.c_str());
On 2012/11/05 19:07:15, abarth wrote:
> Where does event_name come from?  Could this lead to XSS?

I have changed this code to use WebDOMCustomEvent instead. But if you think
taking an enum as an input-parameter and deciding the event-name from that would
still be better, I can make that change.

http://codereview.chromium.org/11368071/diff/2006/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:329: v8::Local<v8::Object>
event = value->ToObject();
On 2012/11/05 19:07:15, abarth wrote:
> Why not use the WebKit API to create a CustomEvent?  It seems like a hack to
use
> eval to do it.

Done: https://bugs.webkit.org/show_bug.cgi?id=101293

[sadrul, 2012-11-07 03:37:06]

Hi Adam. I am experimenting with a slightly changed approach:
 * Create a generic Event using createEvent("Event")
 * Expose the v8 object for the event (I noticed that this is done for some
other Web wrappers, e.g. WebBlob)
 * Install accessors for the desired attributes. If I understand correctly (and
perhaps I misunderstand), this will not trigger any JS setters?

This way, we do not use CustomEvents, and avoid the JS setters being triggered
when constructing the event. What do you think?

http://codereview.chromium.org/11368071/diff/15001/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

http://codereview.chromium.org/11368071/diff/15001/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:330: WebKit::WebDOMCustomEvent
event = dom_event.to<WebKit::WebDOMCustomEvent>();
Ignore, for the moment, the fact that this is still WebDOMCustomEvent

[abarth-chromium, 2012-11-07 03:39:30]

On 2012/11/07 03:37:06, sadrul wrote:
> Hi Adam. I am experimenting with a slightly changed approach:
>  * Create a generic Event using createEvent("Event")
>  * Expose the v8 object for the event (I noticed that this is done for some
> other Web wrappers, e.g. WebBlob)

We haven't been happy with that decision for WebBlob.  The problem is there
isn't a one-to-one mapping between WebEvents and v8 objects---there's a
one-to-many relationship (due to isolated worlds).

[sadrul, 2012-11-07 17:50:46]

On 2012/11/07 03:39:30, abarth wrote:
> On 2012/11/07 03:37:06, sadrul wrote:
> > Hi Adam. I am experimenting with a slightly changed approach:
> >  * Create a generic Event using createEvent("Event")
> >  * Expose the v8 object for the event (I noticed that this is done for some
> > other Web wrappers, e.g. WebBlob)
> 
> We haven't been happy with that decision for WebBlob.  The problem is there
> isn't a one-to-one mapping between WebEvents and v8 objects---there's a
> one-to-many relationship (due to isolated worlds).

Aah, I think I understand [a little] now. Thanks for explaining!

[sadrul, 2012-11-09 19:36:30]

Hi! I have updated the code according to the offline discussion, and updated the
CL description to explain the approach. Please take a look and see if this looks
reasonable.

[abarth-chromium, 2012-11-09 19:44:34]

This patch is definitely an improvement.  It should clean up the resource leaks.
 We still have one issue with re-entrancy, but maybe we should work on that in
the next patch?

We probably need a way to create a SerializedScriptValue from a base::Value
without going though V8.

I'm not sure about the -internal- naming convention for the events, but it
doesn't seem harmful.

https://codereview.chromium.org/11368071/diff/5009/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11368071/diff/5009/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:332:
detail->Set(v8::String::New(iter->first.data(), iter->first.size()),
This line of code can execute arbitrary JavaScript, which can lead to the
BrowserPlugin being destroyed.  You don't appear to handle that condition
safely.

[abarth-chromium, 2012-11-09 19:44:48]

LGTM with the caveats above.

[abarth-chromium, 2012-11-09 19:45:30]

Can base::Value get serialized to a string?  Perhaps we should send the JSON
string to the JavaScript, which can then call JSON.parse to recover the object? 
In any case, that's something we can worry about in the next CL.

[Fady Samuel, 2012-11-09 19:49:11]

https://codereview.chromium.org/11368071/diff/5009/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11368071/diff/5009/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:332:
detail->Set(v8::String::New(iter->first.data(), iter->first.size()),
On 2012/11/09 19:44:34, abarth wrote:
> This line of code can execute arbitrary JavaScript, which can lead to the
> BrowserPlugin being destroyed.  You don't appear to handle that condition
> safely.

We have a test for deleting BrowserPlugins inside event listeners.

BrowserPlugin doesn't get deleted immediately. Instead, BrowserPlugin::destroy()
gets called which calls:

MessageLoop::current()->DeleteSoon(FROM_HERE, this);

This means is BrowserPlugin will exist for the lifetime of execution of this
method.

I *believe* destroying the BrowserPlugin is not an issue, but there might be
another wacky way to delete the BrowserPlugin that I don't know of.

[abarth-chromium, 2012-11-09 19:52:12]

https://codereview.chromium.org/11368071/diff/5009/content/renderer/browser_p...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/11368071/diff/5009/content/renderer/browser_p...
content/renderer/browser_plugin/browser_plugin.cc:339: WebKit::WebDOMEvent
dom_event = frame->document().createEvent("CustomEvent");
Certainly |frame| can be destroyed, which means we'll have trouble here.

[sadrul, 2012-11-09 21:14:15]

Thanks for the quick review! I have addressed the issue about JS callbacks
destroying the plugin/frame by checking the container and the frame after
setting all the props on the v8 object.

This CL depends on the webkit patches to land first. So as the webkit patches
land and are gardened in, I will start looking at creating the
WebSerializedScriptValue out of the base::Values directly instead of using a v8
value. If I can get that to work before the webkit patches are gardened in, I
will include that change in this patch. Otherwise, I will put up a new patch for
that.

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

http://codereview.chromium.org/11368071/diff/5009/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:339: WebKit::WebDOMEvent
dom_event = frame->document().createEvent("CustomEvent");
On 2012/11/09 19:52:13, abarth wrote:
> Certainly |frame| can be destroyed, which means we'll have trouble here.

I have added a NULL check for container() and frame here.

[sadrul, 2012-11-09 21:53:10]

+creis@ for content/ OWNERS

+sky@ for chrome/ OWNERS

[sky, 2012-11-09 22:50:00]

I'm not a good reviewer for the js changes. Could you get someone else?

[sadrul, 2012-11-09 23:02:53]

+arv@ for reviewing the change in web_view.js

[sadrul, 2012-11-09 23:45:05]

Merged the reviewed changes from http://codereview.chromium.org/11377089/ into
this CL to reduce OWNER review-overhead.

[Charlie Reis, 2012-11-12 18:23:11]

This looks like it will be a great improvement.  A few comments and questions
below.

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

http://codereview.chromium.org/11368071/diff/3017/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:365: if (!container())
Do you still need this if you have the more elaborate check below?

http://codereview.chromium.org/11368071/diff/3017/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:372: end = props->end(); iter
!= end; ++iter) {
nit: This should be indented 4 from the open paren.

http://codereview.chromium.org/11368071/diff/3017/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:381: // Setting properties on
the |detail| object may have triggered JS code that
What |detail| object?

http://codereview.chromium.org/11368071/diff/3017/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:389: std::string
obfuscated_name = base::StringPrintf("-internal-%s",
Can you explain more about why we need obfuscated names, and what this
"-internal-" prefix means?

http://codereview.chromium.org/11368071/diff/3017/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:536: props[kReason] =
base::Value::CreateStringValue(termination_status);
Hmm, is "type" the proper label for event names?  Guess it's a good thing we can
still change the API.

http://codereview.chromium.org/11368071/diff/3017/content/test/data/browser_p...
File content/test/data/browser_plugin_embedder.html (right):

http://codereview.chromium.org/11368071/diff/3017/content/test/data/browser_p...
content/test/data/browser_plugin_embedder.html:8: document.title =
JSON.parse(evt.detail).reason;
I'm trying to understand what we're exposing to real pages.

It looks like the webview shim will expose things like event.url for event names
like "loadabort", while the browser plugin will expose event.detail={"url":url}
for event names like "-internal-loadabort".  Is that right?

Are we concerned about the internal versions being exposed to real pages, or is
that something we can prevent?  Is this conversion documented anywhere?

[sadrul, 2012-11-12 19:00:16]

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

http://codereview.chromium.org/11368071/diff/3017/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:365: if (!container())
On 2012/11/12 18:23:12, creis wrote:
> Do you still need this if you have the more elaborate check below?

Good catch. I have moved the check from below to here instead.

http://codereview.chromium.org/11368071/diff/3017/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:372: end = props->end(); iter
!= end; ++iter) {
On 2012/11/12 18:23:12, creis wrote:
> nit: This should be indented 4 from the open paren.

Done.

http://codereview.chromium.org/11368071/diff/3017/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:381: // Setting properties on
the |detail| object may have triggered JS code that
On 2012/11/12 18:23:12, creis wrote:
> What |detail| object?

Stale comment. Removed.

http://codereview.chromium.org/11368071/diff/3017/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:389: std::string
obfuscated_name = base::StringPrintf("-internal-%s",
On 2012/11/12 18:23:12, creis wrote:
> Can you explain more about why we need obfuscated names, and what this
> "-internal-" prefix means?

'obfuscated' is less correct. I have changed it to 'internal' and added a
comment explaining the reasoning.

http://codereview.chromium.org/11368071/diff/3017/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:536: props[kReason] =
base::Value::CreateStringValue(termination_status);
On 2012/11/12 18:23:12, creis wrote:
> Hmm, is "type" the proper label for event names?  Guess it's a good thing we
can
> still change the API.

Indeed.

http://codereview.chromium.org/11368071/diff/3017/content/test/data/browser_p...
File content/test/data/browser_plugin_embedder.html (right):

http://codereview.chromium.org/11368071/diff/3017/content/test/data/browser_p...
content/test/data/browser_plugin_embedder.html:8: document.title =
JSON.parse(evt.detail).reason;
On 2012/11/12 18:23:12, creis wrote:
> I'm trying to understand what we're exposing to real pages.
> 
> It looks like the webview shim will expose things like event.url for event
names
> like "loadabort", while the browser plugin will expose
event.detail={"url":url}
> for event names like "-internal-loadabort".  Is that right?

That is correct.

> Are we concerned about the internal versions being exposed to real pages, or
is
> that something we can prevent?  Is this conversion documented anywhere?

I think the main concern is to expose a semi-stable documented API to <webview>
consumers (i.e. 'loadabort' -> event.url, etc). The internal events (i.e.
'-internal-' -> event.detail) are implementation details that can change, and
should not be used externally (i.e. undocumented).

[Charlie Reis, 2012-11-12 19:11:13]

LGTM.

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

http://codereview.chromium.org/11368071/diff/3017/content/renderer/browser_pl...
content/renderer/browser_plugin/browser_plugin.cc:389: std::string
obfuscated_name = base::StringPrintf("-internal-%s",
On 2012/11/12 19:00:16, sadrul wrote:
> On 2012/11/12 18:23:12, creis wrote:
> > Can you explain more about why we need obfuscated names, and what this
> > "-internal-" prefix means?
> 
> 'obfuscated' is less correct. I have changed it to 'internal' and added a
> comment explaining the reasoning.

Thanks, that's much clearer.

[sadrul, 2012-11-12 21:39:10]

ping arv@

[arv (Not doing code reviews), 2012-11-12 22:58:07]

http://codereview.chromium.org/11368071/diff/20001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

http://codereview.chromium.org/11368071/diff/20001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:29: 'exit' : [ 'processId',
'reason' ],
no ws after [ nor before ]

http://codereview.chromium.org/11368071/diff/20001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:123: for (eventname in
WEB_VIEW_EVENTS) {
eventName

http://codereview.chromium.org/11368071/diff/20001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:123: for (eventname in
WEB_VIEW_EVENTS) {
missing var

You should really use strict mode for these things. Less risk for security
issues as well.

http://codereview.chromium.org/11368071/diff/20001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:162:
attribs.forEach(function(attribname) {
attribName

http://codereview.chromium.org/11368071/diff/20001/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/web_view/main.js (right):

http://codereview.chromium.org/11368071/diff/20001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view/main.js:140:
webview.addEventListener('loadcommit', function(e) {
webview.addEventListener('loadcommit' loadCommitCommon);

http://codereview.chromium.org/11368071/diff/20001/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view/main.js:146:
webview.setAttribute('src', url);
is there no src setter?

[sadrul, 2012-11-12 23:03:21]

http://codereview.chromium.org/11368071/diff/20001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_view.js (right):

http://codereview.chromium.org/11368071/diff/20001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:29: 'exit' : [ 'processId',
'reason' ],
On 2012/11/12 22:58:07, arv wrote:
> no ws after [ nor before ]

Done.

http://codereview.chromium.org/11368071/diff/20001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:123: for (eventname in
WEB_VIEW_EVENTS) {
On 2012/11/12 22:58:07, arv wrote:
> eventName

Done.

http://codereview.chromium.org/11368071/diff/20001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:123: for (eventname in
WEB_VIEW_EVENTS) {
On 2012/11/12 22:58:07, arv wrote:
> missing var

Done.

> You should really use strict mode for these things. Less risk for security
> issues as well.

It doesn't look like any of the scripts in this location use the strict mode. I
do not know if that is deliberate or not.

http://codereview.chromium.org/11368071/diff/20001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_view.js:162:
attribs.forEach(function(attribname) {
On 2012/11/12 22:58:07, arv wrote:
> attribName

Done.

[arv (Not doing code reviews), 2012-11-12 23:18:37]

JS LGTM

[sadrul, 2012-11-12 23:19:42]

Thanks! sky, can I please have an OWNER stamp :)

[sky, 2012-11-13 00:55:57]

Rubber stamp LGTM