[Webkit-unassigned] [Bug 107207] [V8] Support selectively wrapping DOM accesses from certain V8 contexts.

[bugzilla-daemon at webkit.org]

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





--- Comment #20 from Ulfar Erlingsson <ulfar.chromium at gmail.com>  2013-03-13 13:35:39 PST ---
File Edit Options Buffers Tools Help                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                 
(In reply to comment #19)
> (In reply to comment #17)
> > I just uploaded a new patch, and performance data, which indicates that the cost of wrapping is about the same as an access to an undefined DOM property.
>
> Thanks for the performance data! Specifically, when you enable the wrapping on Node.firstChild and you don't observe any performance regression in PerformanceTests/Bindings/first-child.html, then everything is fine. If you observe slight performance regression, that's negotiable:) Would you check it?

In our application, there is zero performance overhead on Node.firstChild.  And, where it is applied, this mechanism should have less or equal overhead than any alternative mechanism.  See below.

> > Regarding why this is needed, the mechanism is designed to support logging the access to a select set of (security-critical) DOM element properties from a select set of V8 worlds/contexts--in particular, from Chrome extensions.  The set of elements to log, and what to log from what contexts, may change, and possibly be under user control (to some extent) in the future.
> >
> > Apart from some simple plumbing, the mechanism is completely contained within the cover wrapping files.  Any complexity (e.g., how to wrap, what to wrap, what state to track, etc.) should be isolated to that part of the code.
>
> My point is that we don't want to introduce an extra indirection layer to getters/setter/methods. How about this approach?:
>
> - Add a new IDL attribute, say [V8Logging]
> - For getters/setters/methods with [V8Logging], CodeGenerator can insert logging code at the head of xxxAttrGetterCallback(), xxxAttrSetterCallback() and xxxMethodCallback().

Adding an IDL attribute seems like a good idea, irrespective of anything else.

If everybody feels it's a more acceptable approach,  I will modify the Perl script to insert logging code at the head of xxxAttrGetterCallback(), etc.  However, I actually started off with such an implementation, and backed away.  I'll explain below.

First, to clarify, just in case there is some confusion.  Our target is to add activity logging to a handful (few dozen, at most, we imagine) DOM element methods and accessors---only when they are used from code in certain V8 Contexts.  We do want to log access to a couple of possibly common DOM methods/properties, namely createElement, and innerHTML, to spot when extensions add scripts.  Therefore, we wanted to avoid adding *any* performance overhead to those methods in the page's main world and context.  (But, notably, we envision no scenario where we would log methods like firstChild.)

There are two options for inserting code at the head of the callback functions:

A) When adding logging code to the head of createElementMethodCallback, an if statement is needed: activity logging is only to occur for certain contexs, so its necessary to get and examine the current context.  There's a seperate ActivityLogger for each context, so it's possible have the if statement check a pointer (e.g., added to PerContextData) that also get the correct ActivityLogger instance.

B) A simple if statement would add a function call and memory access to the DOM createElement method, even in the main world, for all pages.  An alternative is to put all the logging code into a function createElementMethodCallbackWithLogging, and move the if statement into V8DOMConfiguration, so it sometimes configures xxxWithLogging callbacks when & where we want logging.  To eliminate the if statement code in the main world, this has to be coupled with seperate template maps in PerIsolateData (like in patches Marja and Ankur have been working on).

Option (A) has a negative perf implication on all JavaScript code executed.  Option (B) adds a function call (to xxxWithLogging) and access to state (e.g., from(v8::context::GetCurrent())->logger).

Like the current proposed patch, both options (A) and (B) add function call(s) and a memory access for each of the logged callback functions.  So neither option should do any better in the bindings performance tests.

I explored option (B) and found that it required a significant amount of plumbing.  New xxxWithWrapping functions had to be generated and plumbed into tables to make them accessible to V8DOMConfiguration.  This affected both the Perl script and for the relevant custom wrappers (like document.location).  The current callback mechanism has far less plumbing (just the infoAddr changes and visibleInterfaceName forwarding).

Finally, we are not quite sure what DOM activity needs to be logged: this set is certain change over time, and the set may even be configurable by users or administrators.  There will be a UI in Chrome for viewing the activity and performance impact of extensions.  Adam Barth had suggested that we have the set be determined by strings sent over from Chrome, e.g., configurable by this UI.  This we can easily support with the current closure mechanism, but it would be hard to support with either option (A) or (B) above.

So, I implemented the generic closure mechanism in the current patch.  It adds an indirection only where needed, and should incur performance penalty less or equal to either of the above options, without any significant changes in the V8 bindings code structure.

Closures are generally useful, so having such a mechanism may help implement other features.  That's why I pointed you at this patch: for chrome://tracing, it may be possible to eliminate any if statements like those in option (A), by using closures.

This said, I am quite willing to revert to a patch based on either option (A) or (B) above, if those with more WebKit experience than I see that as the best approach.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.