Bug 107207 – [V8] Support selectively wrapping DOM accesses from certain V8 contexts.

• Component: WebCore JavaScript
• Status: RESOLVED WONTFIX

[Ulfar Erlingsson]

Support selectively wrapping DOM accesses from certain V8 contexts.

[Ulfar Erlingsson]

Created attachment 183488 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 183488 [details] did not pass style-queue:

Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:47:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:52:  The parameter name "info" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:53:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:53:  The parameter name "info" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:61:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:66:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:63:  The parameter name "policy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:67:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:69:  The parameter name "policy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:71:  The parameter name "attribute" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.h:71:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:40:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:44:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:49:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:54:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:56:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:59:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:67:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:74:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:90:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:96:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:127:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8PerContextData.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/v8/V8PerContextData.h:109:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8PerContextData.h:109:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8PerContextData.h:127:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.h:43:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.h:46:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.h:45:  The parameter name "policy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.h:52:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.h:55:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.h:54:  The parameter name "policy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.h:56:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:32:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:52:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:55:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:56:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:62:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:63:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:109:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:110:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:111:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:112:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:115:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:116:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:116:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:117:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:117:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:118:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:119:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:124:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:132:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:133:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:137:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:137:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:138:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:139:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:140:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:141:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:144:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:145:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebKit/chromium/src/WebDOMCoverWrapping.cpp:31:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/Platform/chromium/public/WebDOMCoverWrapping.h:35:  WEBKIT_EXPORT should not be used on a function with a body.  [readability/webkit_export] [5]
Source/Platform/chromium/public/WebDOMCoverWrapping.h:36:  WEBKIT_EXPORT should not be used on a function with a body.  [readability/webkit_export] [5]
Source/Platform/chromium/public/WebDOMCoverWrapping.h:37:  WEBKIT_EXPORT should not be used on a function with a body.  [readability/webkit_export] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:32:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:33:  Found header this file implements after a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:60:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:62:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:65:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:66:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:96:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:102:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:116:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:117:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:118:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/chromium/public/WebDOMCove..." exit_code: 1

Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:119:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:120:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:121:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:123:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:130:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:138:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:142:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:147:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:148:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:156:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:158:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:159:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMConfiguration.h:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMConfiguration.h:67:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMConfiguration.h:68:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMConfiguration.h:69:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMConfiguration.h:102:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/v8/V8DOMConfiguration.h:119:  The parameter name "attribute" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/v8/V8DOMConfiguration.h:119:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 107 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ulfar Erlingsson]

Support "cover wrapping" to modify select DOM accesses from select V8 contexts. 

A particular use of cover wrapping is to address bugs like BUG 160989, which 
require logging DOM access from certain contexts (e.g., Javascript extensions). 

Another use could be to easily support stats-gathering like that of the 
recently eliminated INC_STATS. 

The cover wrapping mechanisms allow a V8-based closure to be selectively 
substited for any DOM-access function pointer provided in the template tables 
that configure the DOM objects accessible to a V8 execution context. 

Notably, those tables (V8DOMConfiguration::BatchedAttribute) are automatically 
generated from a Perl script that is changed to provide an extra indirection to 
any info arguments (by using a new WrapperTypeInfo** infoAddr variable). This 
change enables cover wrapper closures to make use of the implicitly-provided 
arguments args.Data() and info.Data() to each V8 accessor or method callback.

[Ulfar Erlingsson]

NOTE: Currently just seeking feedback on the overall approach.

Will fix the coding-guideline bugs, add tests, etc., assuming the overall approach seems reasonable.  (Suggestions as to how to do the tests would be appreciated: Are there WebKit tests that check for correct console output?)

[Adam Barth]

Comment on attachment 183488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183488&action=review

> Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:99
> +        v8::Handle<v8::FunctionTemplate> callbackTemplate = v8::FunctionTemplate::New(callback.callback, v8Undefined(), signature);

This looks like a memory leak.  Where is the matching call to Dispose?

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:71
> +    v8::HandleScope scope;

It's very unlikely that you need a HandelScope here given that you're taking a v8::Local as a parameter.

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:82
> +    delete instanceData->m_wrapper;
> +    delete instanceData;

We try to avoid calling "delete" manually.  Instead, we use OwnPtr and related classes to do this work automatically.

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:125
> +    v8::Persistent<v8::Value> dataHandle = v8::Persistent<v8::Value>::New(v8::External::New(instanceData));

Looks like another memory leak.

> Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:103
> +        logEntryFormatStr->append(className);

Please use StringBuilder rather than calling "append" on a String.

> Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:112
> +// TODO: Figure out a way to make these policies be robust wrt the IDL files, etc.

TODO -> FIXME

[Adam Barth]

Comment on attachment 183488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183488&action=review

> Source/WebCore/bindings/v8/V8DOMCoverWrappingPolicy.cpp:67
> +    OwnPtr<WTF::String> m_logString;

There's no point in having an OwnPtr to a String.  You should just hold the String directly.  It's just a stack-allocated object that points to a reference counted object that holds the underlying string data.

[Adam Barth]

It's hard for me to understand what this patch is doing for two reasons:

A) All the policy-related code obscures the actual meat of the wrapping and logging mechanism.
B) All the style and memory errors are very distracting.

What I'd recommend is to break this change down into a number of smaller steps:

1) The first patch would contain the wrapping mechanism, but we would not actually wrap anything.
2) The second patch would contain the policy mechanism for controlling the wrapping.

Please also run check-webkit-style before uploading patches.  The reason we have a style guide is to make it easier for people to understand code written by other people.  You'll make it easier for me to understand your code if you follow the style guide.

It's also likely that we'll need to do a few rounds on each patch to clean up the memory errors.  I can understand if you'd like feedback on the general approach before putting in the detailed work to manage memory correctly, but it's difficult for me to see the larger approach with all these errors.  To give a musical analogy, it's like asking someone what they think of a song playing on the radio when the radio has a lot of static.  In principle, you should be able to appreciate the song separately from the static, but the static is really distracting.

For the first patch, I would encourage you to focus on the core wrapping mechanism, assuming that's the part you're most interested in feedback about.

[Ulfar Erlingsson]

(In reply to comment #8)
> It's hard for me to understand what this patch is doing for two reasons:
> 
> A) All the policy-related code obscures the actual meat of the wrapping and logging mechanism.
> B) All the style and memory errors are very distracting.
> 
> What I'd recommend is to break this change down into a number of smaller steps:
> 
> 1) The first patch would contain the wrapping mechanism, but we would not actually wrap anything.
> 2) The second patch would contain the policy mechanism for controlling the wrapping.
>

Thanks for this feedback.  This is exactly the type of guidance I was hoping for.

> Please also run check-webkit-style before uploading patches.  The reason we have a style guide is to make it easier for people to understand code written by other people.  You'll make it easier for me to understand your code if you follow the style guide.
> 

Yes.  Was developing this in the Chromium tree, so hadn't seen the style problems until just submitting the bug.  Will convert to a tree based on the WebKit pull, and keep things clean.

> It's also likely that we'll need to do a few rounds on each patch to clean up the memory errors.  I can understand if you'd like feedback on the general approach before putting in the detailed work to manage memory correctly, but it's difficult for me to see the larger approach with all these errors.  To give a musical analogy, it's like asking someone what they think of a song playing on the radio when the radio has a lot of static.  In principle, you should be able to appreciate the song separately from the static, but the static is really distracting.
> 

Apologies for the memory management bugs (aka static noise).  I had made a significant effort to make the code leak free (e.g., by using MakeWeak), but I had failed to enable valgrind/heapcheck end-to-end detection.  I must also work on my understanding of the V8/bindings memory management (e.g., why the current prototype->Set(..., v8::FunctionTemplate::New(...)) in batchConfigureCallbacks doesn't leak memory, but assigning to a v8::Handle may).  

> For the first patch, I would encourage you to focus on the core wrapping mechanism, assuming that's the part you're most interested in feedback about.

Yes.  Thanks again for taking the time to look at this.

[Ulfar Erlingsson]

Created attachment 191309 [details]
Patch

[Kentaro Hara]

Comment on attachment 191309 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191309&action=review

Overall the approach looks good to me.

I'm interested in performance impacts when you enable the wrapping? Did you observe performance impacts on Dromaeo/dom-*.html or Bindings/* ?

> Source/WebCore/ChangeLog:7
> +

Please describe what your change is doing.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You need to update run-bindings-tests.

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:104
> +    v8::Persistent<v8::Value> dataHandle = v8::Persistent<v8::Value>::New(isolate, v8::External::New(instanceData));
> +    dataHandle.MakeWeak(isolate, instanceData, weakReferenceCallback);

Can't you use ScopedPersistent to manage the lifetime? (although the lifetime will be almost the same as the lifetime of WebKit.)

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:107
> +    // Override the method callback, redirecting it to the coverwrapping dispatcher.
> +    callbackTemplate->SetCallHandler(dispatchCoverWrappedMethod, dataHandle);

It's a bit inconsistent that you are setting callbacks in V8DOMCoverWrapping.cpp whereas you are setting getters/setters in V8DOMConfigurations.cpp. Could you make it more consistent?

> Source/WebCore/bindings/v8/WrapperTypeInfo.h:78
> +        static WrapperTypeInfo* unwrapAddr(v8::Handle<v8::Value> typeInfoAddrWrapper)

Nit: "unwrapInfoAddr" would be a better name.

[Ulfar Erlingsson]

(In reply to comment #11)
> (From update of attachment 191309 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191309&action=review
> 
> Overall the approach looks good to me.

That's great to hear.

> I'm interested in performance impacts when you enable the wrapping? Did you observe performance impacts on Dromaeo/dom-*.html or Bindings/* ?

I'll run the performance tests and get those numbers today.  I don't expect there to be any measurable overhead, since there are only a few extra machine instructions plus one extra memory access per DOM wrapper invocation. 

Note: If the perf cost is as low as I expect, this mechanism can be used for various types of optional monitoring---such as gathering of trace data---without any cost being incurred by normal operation.

> > Source/WebCore/ChangeLog:7
> > +
>
> Please describe what your change is doing.

Will add.
 
> > Source/WebCore/ChangeLog:8
> > +        No new tests (OOPS!).
> 
> You need to update run-bindings-tests.
> 

Will do.  Sorry, I'm new to this.

> > Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:104
> > +    v8::Persistent<v8::Value> dataHandle = v8::Persistent<v8::Value>::New(isolate, v8::External::New(instanceData));
> > +    dataHandle.MakeWeak(isolate, instanceData, weakReferenceCallback);
> 
> Can't you use ScopedPersistent to manage the lifetime? (although the lifetime will be almost the same as the lifetime of WebKit.)

Instead of using V8 Persistent and weak callbacks, it would be cleaner to pass ownership of all the closure data instances to an array in V8PerIsolateData.  What do you think?

> > Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:107
> > +    // Override the method callback, redirecting it to the coverwrapping dispatcher.
> > +    callbackTemplate->SetCallHandler(dispatchCoverWrappedMethod, dataHandle);
> 
> It's a bit inconsistent that you are setting callbacks in V8DOMCoverWrapping.cpp whereas you are setting getters/setters in V8DOMConfigurations.cpp. Could you make it more consistent?
> 

Yes, I was also annoyed by this inconsistency, which came about because the V8 method callback data parameter can't be installed during the V8 Set Call (like with the SetAccessor API), but only afterwards with a SetCallHandler.  I'll try to clean up.

> > Source/WebCore/bindings/v8/WrapperTypeInfo.h:78
> > +        static WrapperTypeInfo* unwrapAddr(v8::Handle<v8::Value> typeInfoAddrWrapper)
> 
> Nit: "unwrapInfoAddr" would be a better name.

WIll do.

[Kentaro Hara]

> > I'm interested in performance impacts when you enable the wrapping? Did you observe performance impacts on Dromaeo/dom-*.html or Bindings/* ?
> 
> I'll run the performance tests and get those numbers today.  I don't expect there to be any measurable overhead, since there are only a few extra machine instructions plus one extra memory access per DOM wrapper invocation. 
> 
> Note: If the perf cost is as low as I expect, this mechanism can be used for various types of optional monitoring---such as gathering of trace data---without any cost being incurred by normal operation.

hmm, I begin to wonder why this change is needed...:) It looks like the change is going to add unnecessary complexity to the code base. Also it is going to add "a few extra machine instructions plus one extra memory access", which sounds heavy (although we should judge it based on performance measurement).

What you want to do is adding wrapper methods for getters/setters/methods (selectively). Currently, xxxAttrGetterCallback(), xxxAttrSetterCallback() and xxxMethodCallback() are the entry points from V8. How about just hooking those entry points in code generators?

[Ulfar Erlingsson]

Created attachment 191857 [details]
Patch

[WebKit Review Bot]

Attachment 191857 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/scripts/test/V8/V8Float64Array.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8Float64Array.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestCustomNamedGetter.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestCustomNamedGetter.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventConstructor.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventConstructor.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventTarget.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventTarget.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestException.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestException.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestInterface.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestMediaQueryListListener.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestMediaQueryListListener.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestNode.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestNode.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.h', u'Source/WebCore/bindings/v8/V8DOMConfiguration.cpp', u'Source/WebCore/bindings/v8/V8DOMConfiguration.h', u'Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp', u'Source/WebCore/bindings/v8/V8DOMCoverWrapping.h', u'Source/WebCore/bindings/v8/WrapperTypeInfo.h', u'Source/WebCore/bindings/v8/custom/V8HTMLImageElementConstructor.cpp', u'Source/WebCore/bindings/v8/custom/V8HTMLImageElementConstructor.h']" exit_code: 1
Source/WebCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/bindings/v8/V8DOMConfiguration.h:68:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ulfar Erlingsson]

Created attachment 191860 [details]
Adding JSON data for perf overhead numbers.

High-order bit seems to be that a coverwrapping closure callback has about the same cost as an access to an undefined DOM element property.

[Ulfar Erlingsson]

(In reply to comment #13)
> > > I'm interested in performance impacts when you enable the wrapping? Did you observe performance impacts on Dromaeo/dom-*.html or Bindings/* ?
> > 
> > I'll run the performance tests and get those numbers today.  I don't expect there to be any measurable overhead, since there are only a few extra machine instructions plus one extra memory access per DOM wrapper invocation. 
> > 
> > Note: If the perf cost is as low as I expect, this mechanism can be used for various types of optional monitoring---such as gathering of trace data---without any cost being incurred by normal operation.
> 
> hmm, I begin to wonder why this change is needed...:) It looks like the change is going to add unnecessary complexity to the code base. Also it is going to add "a few extra machine instructions plus one extra memory access", which sounds heavy (although we should judge it based on performance measurement).
> 

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.

Still unresolved is the memory leak.  I'm hoping ABarth can help me fix that, assuming everything else passes muster.

Regarding performance, note that this patch is designed to incur zero overhead except on DOM element properties that are being "cover wrapped".  In our initial application, that set will be very small, and cover wrapping will only be applied at all in the V8 contexts of extensions.

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.

> What you want to do is adding wrapper methods for getters/setters/methods (selectively). Currently, xxxAttrGetterCallback(), xxxAttrSetterCallback() and xxxMethodCallback() are the entry points from V8. How about just hooking those entry points in code generators?

We're happy to consider any code changes that support the type of Chrome extension activity logging we'd like to add.  And the simpler the better.  I previously did look at modifying the code generation Perl script (initially targeting the INC_STATS macro locations), but that patch got unwieldy and required significant changes to plumbing.  However, I can try again, if needed.

[WebKit Review Bot]

Comment on attachment 191857 [details]
Patch

Attachment 191857 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17010035

[Kentaro Hara]

(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?


> 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().

[Ulfar Erlingsson]

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.

[Adam Barth]

Comment on attachment 191857 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191857&action=review

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:81
> +static void weakReferenceCallback(v8::Isolate* isolate, v8::Persistent<v8::Value> value, void* dataPtr)

We have a new mechanism for making weak callbacks typesafe.  We should probably use that here now.

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:85
> +    value.Dispose(isolate);

We usually call value.Clear after disposing the handle.  I'm not sure it actually does much though.

> Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:97
> +    const V8DOMCoverWrapping::ClosureData dataContents = {0, method, 0, 0};
> +    V8DOMCoverWrapping::ClosureData* instance = new V8DOMCoverWrapping::ClosureData(dataContents);

This is a strange way of creating a V8DOMCoverWrapping::ClosureData object.  Why do we have the const one?

[Kentaro Hara]

Thank you very much for the detailed explanation!

I might want to understand your situation a bit more (I'm not intending to object to your proposal). My primary concern is that getters/setters/methods in V8 bindings are already complicated and thus we don't want to make them more complicated without strong reasons. In terms of logging, we already have FeatureObservers and TRACE_EVENT macros. Note that both TRACE_EVENT macros and FeatureObserers are implemented just by auto-inserting code into the head of getters/setters/methods.

> 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.

I'm not sure how much the mechanism is general. As you mentioned, you're not yet sure how and how much your logging mechanism is going to be used in the future. In addition, I suspect that the mechanism won't be helpful for about:tracing so much. Given that each DOM operation can finish within 20 nano seconds, all we can do in a TRACE_EVENT macro is anyway just to update one global state variable. Thus we don't need something like closures to insert TRACE_EVENT macros, we can just insert TRACE_EVENT macros at the head of getters/setters/methods. On the other hand, FeatureObservers might be benefited by the mechanism(, although FeatureObservers intend to log DOM operations in _all_ worlds). 

Marja is currently implementing two templates for all interfaces: one for the main world and the other for isolated worlds. How about waiting for the work and then implementing a mechanism that selectively auto-inserts logging code into the head of getters/setters/methods of the templates of the world you are interested in? ('Selectively' means that you can control getters/setters/methods and worlds to which the logging code is inserted by using IDL attributes.)

[Adam Barth]

Comment on attachment 191857 [details]
Patch

Ulfar and I discussed this patch a bit.  He's going to break it down into some smaller pieces and look at using other approaches for storing the needed information in the data field.

[Ulfar Erlingsson]

(In reply to comment #21)
> (From update of attachment 191857 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191857&action=review
> 
> > Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:81
> > +static void weakReferenceCallback(v8::Isolate* isolate, v8::Persistent<v8::Value> value, void* dataPtr)
> 
> We have a new mechanism for making weak callbacks typesafe.  We should probably use that here now.

what is that mechanism?

> 
> > Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:85
> > +    value.Dispose(isolate);
> 
> We usually call value.Clear after disposing the handle.  I'm not sure it actually does much though.

Couldn't tell myself.  Will call clear.

> 
> > Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:97
> > +    const V8DOMCoverWrapping::ClosureData dataContents = {0, method, 0, 0};
> > +    V8DOMCoverWrapping::ClosureData* instance = new V8DOMCoverWrapping::ClosureData(dataContents);
> 
> This is a strange way of creating a V8DOMCoverWrapping::ClosureData object.  Why do we have the const one?

Idiomatic way of heap-allocating an initialized C++ POD struct in a manner friendly to compiler-optimization (in the 90's).  Will go away with the changes we discussed.

[Ulfar Erlingsson]

(In reply to comment #22)
> Thank you very much for the detailed explanation!
> 
> I might want to understand your situation a bit more (I'm not intending to object to your proposal). My primary concern is that getters/setters/methods in V8 bindings are already complicated and thus we don't want to make them more complicated without strong reasons. In terms of logging, we already have FeatureObservers and TRACE_EVENT macros. Note that both TRACE_EVENT macros and FeatureObserers are implemented just by auto-inserting code into the head of getters/setters/methods.

Totally understand and appreciate the need to avoid greater complexity.

> > 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.
> 
> I'm not sure how much the mechanism is general. As you mentioned, you're not yet sure how and how much your logging mechanism is going to be used in the future. In addition, I suspect that the mechanism won't be helpful for about:tracing so much. Given that each DOM operation can finish within 20 nano seconds, all we can do in a TRACE_EVENT macro is anyway just to update one global state variable. Thus we don't need something like closures to insert TRACE_EVENT macros, we can just insert TRACE_EVENT macros at the head of getters/setters/methods. On the other hand, FeatureObservers might be benefited by the mechanism(, although FeatureObservers intend to log DOM operations in _all_ worlds). 

A closure mechanism can be useful in many cases other than logging.  In particular, whenever there is optional modification to the behavior of a callback, and you don't want to pay any cost in the normal, unmodified case.  What is the cost of the TRACE_EVENT macros when chrome://tracing is not being used?

> Marja is currently implementing two templates for all interfaces: one for the main world and the other for isolated worlds. How about waiting for the work and then implementing a mechanism that selectively auto-inserts logging code into the head of getters/setters/methods of the templates of the world you are interested in? ('Selectively' means that you can control getters/setters/methods and worlds to which the logging code is inserted by using IDL attributes.)

There is a lot of overlap with that, and option (B) that I outlined above.  Is there some version of the patch that I can see?

[Kentaro Hara]

Discussed with Ulfar and Ataly offline. Given that marja is doing essentially the same work and her work will simplify the logging mechanism, we will wait for marja's work.

[Dan Carney]

> 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.


Any dynamic modification of callbacks disables a large number of the future optimizations we have in the pipeline.  Specifically, instant context startup is made difficult or impossible.  This is somewhat mitigated by using IDL attributes, although it will be a lot more work.

Also, any instrumented callback cannot be inlined by the V8 compiler.

[Kentaro Hara]

(In reply to comment #27)
> > 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.
> 
> 
> Any dynamic modification of callbacks disables a large number of the future optimizations we have in the pipeline.  Specifically, instant context startup is made difficult or impossible.  This is somewhat mitigated by using IDL attributes, although it will be a lot more work.
> 
> Also, any instrumented callback cannot be inlined by the V8 compiler.

After marja's work is completed, we will just insert logging code into the head of DOM getters/setters/methods of the worlds we are inserted in, which will be controlled by IDL attributes. So I hope that it won't prevent optimizations of V8.

[Dan Carney]

> After marja's work is completed, we will just insert logging code into the head of DOM getters/setters/methods of the worlds we are inserted in, which will be controlled by IDL attributes. So I hope that it won't prevent optimizations of V8.

Yes, I see that.  It remains important though that nothing be dynamic., ie - everything needs to be specified in the IDL.  We pay a very high cost for context startup for all worlds, and we'd like to get rid of that.

[Dan Carney]

(In reply to comment #24)
> (In reply to comment #21)
> > (From update of attachment 191857 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191857&action=review
> > 
> > > Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:81
> > > +static void weakReferenceCallback(v8::Isolate* isolate, v8::Persistent<v8::Value> value, void* dataPtr)
> > 
> > We have a new mechanism for making weak callbacks typesafe.  We should probably use that here now.
> 
> what is that mechanism?
> 
> > 
> > > Source/WebCore/bindings/v8/V8DOMCoverWrapping.cpp:85
> > > +    value.Dispose(isolate);
> > 
> > We usually call value.Clear after disposing the handle.  I'm not sure it actually does much though.
> 

clear() should not be called on the handle passed back in the callback but on the original handle.  either way it's pretty irrelevant here as your data objects will remain strongly reachable by the objecttemplate and never be called back.

[Ulfar Erlingsson]

(In reply to comment #29)
> > After marja's work is completed, we will just insert logging code into the head of DOM getters/setters/methods of the worlds we are inserted in, which will be controlled by IDL attributes. So I hope that it won't prevent optimizations of V8.
> 
> Yes, I see that.  It remains important though that nothing be dynamic., ie - everything needs to be specified in the IDL.  We pay a very high cost for context startup for all worlds, and we'd like to get rid of that.

Just confirming that (In reply to comment #29)
> > After marja's work is completed, we will just insert logging code into the head of DOM getters/setters/methods of the worlds we are inserted in, which will be controlled by IDL attributes. So I hope that it won't prevent optimizations of V8.
> 
> Yes, I see that.  It remains important though that nothing be dynamic., ie - everything needs to be specified in the IDL.  We pay a very high cost for context startup for all worlds, and we'd like to get rid of that.

Ack.  Am preparing an IDL-driven version of this patch, and doing perf measurements, in anticipation of Marja's patch.

[Ulfar Erlingsson]

Created attachment 196060 [details]
Patch

[Kentaro Hara]

Comment on attachment 196060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196060&action=review

r-ing because you don't yet resolve the performance regression caused by V8PerContextData::from(v8::Context::GetCurrent())->activityLogger().

> Source/WebCore/ChangeLog:7
> +

Please describe a rationale for the change.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You need to add a test to run-bindings-tests. See https://trac.webkit.org/wiki/WebKitIDL#RunBindingsTests

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:945
> +    my $accessType = shift;

Let's use "getter", "setter" and "method", instead of "get", "set" and "call".

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:958
> +        OwnArrayPtr<v8::Handle<v8::Value> > loggerArgs = toOwnArrayPtrArguments<v8::Handle<v8::Value> >(args, 0);

Why do you need to use OwnArrayPtr?

Given that values in 'args' don't need to outlive logger->log(), I think you don't need to own them here. (c.f. you are not protecting the lifetime in the "get" case below.)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:963
> +        v8::Handle<v8::Value> loggerArg[] = { value };

You don't own value here. I think this is OK.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1360
> +    if (HasActivityLogging($forMainWorldSuffix, $attrExt, "Getter")) {

"Getter" => "Setter"

> Source/WebCore/bindings/scripts/IDLAttributes.txt:137
> +V8ActivityLog=Access|Setter|Getter|AccessForAllWorlds|SetterForAllWorlds|GetterForAllWorlds

Nit: I'd prefer Access|Setter|Getter|AccessForIsolatedWorlds|SetterForIsolatedWorlds|GetterForIsolatedWorlds. "ForAllWorlds by default" sounds a bit better to me.

> Source/WebCore/bindings/v8/V8Binding.h:399
> +    PassOwnArrayPtr<T> toOwnArrayPtrArguments(const v8::Arguments& args, int startIndex)

Nit: Will startIndex be helpful?

[Ulfar Erlingsson]

Created attachment 196263 [details]
Patch

[Kentaro Hara]

Comment on attachment 196263 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196263&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:958
> +        OwnArrayPtr<v8::Handle<v8::Value> > loggerArgs = toOwnArrayPtrArguments<v8::Handle<v8::Value> >(args);

Would you explain why you need to use OwnArrayPtr?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:976
> +    V8DOMActivityLogger* logger = V8PerContextData::from(v8::Context::GetCurrent())->activityLogger();

The patch looks OK as long as this doesn't regress performance.

Unfortunately, it regresses performance today. I hope that the GetCurrent(Isolate*) patch in V8 will resolve the performance issue. Would you measure how much it helps?

[WebKit Review Bot]

Comment on attachment 196263 [details]
Patch

Attachment 196263 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17304655

New failing tests:
http/tests/xmlhttprequest/detaching-frame-2.html
fast/xmlhttprequest/xmlhttprequest-open-after-iframe-onload-remove-self.html
userscripts/window-onerror-for-isolated-world-2.html
fast/dom/xmlhttprequest-constructor-in-detached-document.html

[WebKit Review Bot]

Created attachment 196287 [details]
Archive of layout-test-results from gce-cr-linux-03 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Ulfar Erlingsson]

Created attachment 196407 [details]
Patch

[Sam Weinig]

V8 is no longer supported in WebKit.