Support for selective DOM activity logging, based on IDL attributes.

Support for selective DOM activity logging, based on IDL attributes.

Add support for activity logging, which logs access to selected DOM
attributes and methods, based on IDL attribute markings.  Logging adds code
to getter, setter, and method callbacks in the V8 bindings, either in all
of the callbacks, or just the callbacks for the isolated worlds.

R=haraken@chromium.org
BUG=160989
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148146

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148183

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148343

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148749

[haraken, 2013-04-11 00:59:16]

Would you double-check Dromaeo/dom-* performance again? If you cannot observe
any regression, I'm happy to go with the patch.

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
Source/bindings/scripts/CodeGeneratorV8.pm:955: my $content = "";
Nit: Remove this.

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
Source/bindings/scripts/CodeGeneratorV8.pm:1704: if
(HasActivityLogging($forMainWorldSuffix,
$function->signature->extendedAttributes, "Access")) {
Nit: "Access" => "Method" (Just for readability. This won't change behavior.)

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
File Source/bindings/scripts/IDLAttributes.txt (right):

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
Source/bindings/scripts/IDLAttributes.txt:96:
ActivityLog=Access|Setter|Getter|AccessForIsolatedWorlds|SetterForIsolatedWorlds|GetterForIsolatedWorlds
Put this line near the head of the file. All IDL attributes in this file should
be alphabetically sorted.

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
File Source/bindings/v8/V8Binding.h (right):

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
Source/bindings/v8/V8Binding.h:398: Vector<T> toVectorOfArguments(const
v8::Arguments& args)
This doesn't need to be a template, since a type of Arguments is always
v8::Handle<v8::Value>.

I cannot consider other use cases of this helper method, so I might want to just
write the logic inline in CodeGeneratorV8.pm (rather than creating a helper
method).

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
Source/bindings/v8/V8Binding.h:401: int length = args.Length();
Nit: int => size_t

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
Source/bindings/v8/V8Binding.h:402: for (int i = 0; i < length; ++i)
Nit: int => size_t

[ulfar, 2013-04-11 03:24:10]

I double-checked Dromaeo/dom-* performance, and there was no measurable
performance regression on my workstation.  

(The numbers mostly stayed the same, although the dom-attr test got faster by 5%
with my patch, and the dom-query got a couple of percent slower with my patch,
with a stddev of about a percent.  In both cases within what could be caused by
cache layout changes and noise on my workstation.)

[ulfar, 2013-04-11 03:25:15]

I double-checked Dromaeo/dom-* performance, and there was no measurable
performance regression on my workstation.  

(The numbers mostly stayed the same, although the dom-attr test got faster by 5%
with my patch, and the dom-query got a couple of percent slower with my patch,
with a stddev of about a percent.  In both cases within what could be caused by
cache layout changes and noise on my workstation.)

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
File Source/bindings/scripts/CodeGeneratorV8.pm (right):

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
Source/bindings/scripts/CodeGeneratorV8.pm:955: my $content = "";
On 2013/04/11 00:59:16, haraken wrote:
> Nit: Remove this.

Done.

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
Source/bindings/scripts/CodeGeneratorV8.pm:1704: if
(HasActivityLogging($forMainWorldSuffix,
$function->signature->extendedAttributes, "Access")) {
I'd rather not, since this is a bit awkward.  There are currently IDL attributes
to log getters, setters, or any type of access: method calls, getters, and
setters.  This is the "Access" attribute.  I'd rather keep just three IDL
attribute types (times two, for isolated worlds), and not add a separate fourth
attribute for "Method".

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
File Source/bindings/scripts/IDLAttributes.txt (right):

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
Source/bindings/scripts/IDLAttributes.txt:96:
ActivityLog=Access|Setter|Getter|AccessForIsolatedWorlds|SetterForIsolatedWorlds|GetterForIsolatedWorlds
On 2013/04/11 00:59:16, haraken wrote:
> Put this line near the head of the file. All IDL attributes in this file
should
> be alphabetically sorted.

Done.

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
File Source/bindings/v8/V8Binding.h (right):

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
Source/bindings/v8/V8Binding.h:398: Vector<T> toVectorOfArguments(const
v8::Arguments& args)
Inlining this might lead to some code size increase, since it would be repeated
a few places.  For now I removed the template argument, and moved the code to
the CPP file.

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
Source/bindings/v8/V8Binding.h:401: int length = args.Length();
On 2013/04/11 00:59:16, haraken wrote:
> Nit: int => size_t

Done.

https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
Source/bindings/v8/V8Binding.h:402: for (int i = 0; i < length; ++i)
On 2013/04/11 00:59:16, haraken wrote:
> Nit: int => size_t

Done.

[haraken, 2013-04-11 03:29:42]

The change looks good. Would you add bindings-tests? You can add
attributes/methods with [ActivityLog=*] to TestObj.idl.

On 2013/04/11 03:25:15, ulfar wrote:
> I double-checked Dromaeo/dom-* performance, and there was no measurable
> performance regression on my workstation.  
> 
> (The numbers mostly stayed the same, although the dom-attr test got faster by
5%
> with my patch, and the dom-query got a couple of percent slower with my patch,
> with a stddev of about a percent.  In both cases within what could be caused
by
> cache layout changes and noise on my workstation.)
> 
>
https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
> File Source/bindings/scripts/CodeGeneratorV8.pm (right):
> 
>
https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
> Source/bindings/scripts/CodeGeneratorV8.pm:955: my $content = "";
> On 2013/04/11 00:59:16, haraken wrote:
> > Nit: Remove this.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
> Source/bindings/scripts/CodeGeneratorV8.pm:1704: if
> (HasActivityLogging($forMainWorldSuffix,
> $function->signature->extendedAttributes, "Access")) {
> I'd rather not, since this is a bit awkward.  There are currently IDL
attributes
> to log getters, setters, or any type of access: method calls, getters, and
> setters.  This is the "Access" attribute.  I'd rather keep just three IDL
> attribute types (times two, for isolated worlds), and not add a separate
fourth
> attribute for "Method".
> 
>
https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
> File Source/bindings/scripts/IDLAttributes.txt (right):
> 
>
https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/scr...
> Source/bindings/scripts/IDLAttributes.txt:96:
>
ActivityLog=Access|Setter|Getter|AccessForIsolatedWorlds|SetterForIsolatedWorlds|GetterForIsolatedWorlds
> On 2013/04/11 00:59:16, haraken wrote:
> > Put this line near the head of the file. All IDL attributes in this file
> should
> > be alphabetically sorted.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
> File Source/bindings/v8/V8Binding.h (right):
> 
>
https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
> Source/bindings/v8/V8Binding.h:398: Vector<T> toVectorOfArguments(const
> v8::Arguments& args)
> Inlining this might lead to some code size increase, since it would be
repeated
> a few places.  For now I removed the template argument, and moved the code to
> the CPP file.
> 
>
https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
> Source/bindings/v8/V8Binding.h:401: int length = args.Length();
> On 2013/04/11 00:59:16, haraken wrote:
> > Nit: int => size_t
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/13799007/diff/6007/Source/bindings/v8/...
> Source/bindings/v8/V8Binding.h:402: for (int i = 0; i < length; ++i)
> On 2013/04/11 00:59:16, haraken wrote:
> > Nit: int => size_t
> 
> Done.

[haraken, 2013-04-11 03:49:59]

Discussed offline. ./run-bindings-tests is not working (again). Later I'll add
binding-tests for you.

LGTM. I'll land your patch in hours.

[commit-bot: I haz the power, 2013-04-11 04:05:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ulfar@google.com/13799007/12001

[commit-bot: I haz the power, 2013-04-11 05:07:05]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-04-11 05:25:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ulfar@google.com/13799007/12001

[commit-bot: I haz the power, 2013-04-11 05:25:41]

Change committed as 148146

[eseidel, 2013-04-11 08:15:21]

This appears to have caused lots of crashes on teh bots?
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...

[eseidel, 2013-04-11 08:15:21]

This appears to have caused lots of crashes on teh bots?
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...

[haraken, 2013-04-11 08:17:12]

Thanks for letting me know. Let me revert it.

[haraken, 2013-04-11 08:21:06]

ulfar: would you take a look at the crashes?

[commit-bot: I haz the power, 2013-04-11 13:39:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ulfar@google.com/13799007/28001

[commit-bot: I haz the power, 2013-04-11 13:39:53]

Change committed as 148183

[haraken, 2013-04-11 13:41:57]

oops, I checked the commit button without removing NOTRY=true line... Anyway,
given that I have to leave soon, let me revert it:)

[haraken, 2013-04-11 13:45:12]

I'll reland the patch tomorrow morning.

[felt, 2013-04-12 02:13:51]

Ulfar, thanks for adding the HTML5 logging so fast!

[ulfar, 2013-04-12 22:03:28]

Kentaro: I added the binding tests, so the patch is hopefully all ready now.  
Haven't seen any problems locally, when running tests, but am running the try
bots just in case.

[haraken, 2013-04-15 01:27:20]

Committed patchset #7 manually as r148343 (presubmit successful).

[haraken, 2013-04-15 04:25:35]

On 2013/04/15 01:27:20, haraken wrote:
> Committed patchset #7 manually as r148343 (presubmit successful).

Rolled out the CL in r148351 because it caused a bunch of failures in layout
tests and browser tests:

http://chromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linux/bui...
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@ToT -
chromium.org&testType=content_browsertests&tests=WorkerXHRHttpLayoutTest.Tests

Ulfar: Would you check them again?

[haraken, 2013-04-15 04:34:53]

On 2013/04/15 04:25:35, haraken wrote:
> Ulfar: Would you check them again?

I'd guess the reason is that V8PerContextData::m_activityLogger is not
initialized to 0.

[commit-bot: I haz the power, 2013-04-18 05:13:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ulfar@google.com/13799007/44001

[commit-bot: I haz the power, 2013-04-18 05:13:08]

Failed to apply patch for Source/WebCore/dom/Document.idl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         Source/WebCore
  Created missing directory Source/WebCore.
  A         Source/WebCore/dom
  Created missing directory Source/WebCore/dom.
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/WebCore/dom/Document.idl
  |diff --git a/Source/WebCore/dom/Document.idl
b/Source/WebCore/dom/Document.idl
  |index
2884c1b55ef79605c4085f382094e85c84e4605b..aa30931a24e54826c54d80ff6edad682dd22be70
100644
  |--- a/Source/WebCore/dom/Document.idl
  |+++ b/Source/WebCore/dom/Document.idl
  --------------------------
  No file to patch.  Skipping patch.
  4 out of 4 hunks ignored

Patch:       Source/WebCore/dom/Document.idl
Index: Source/WebCore/dom/Document.idl
diff --git a/Source/WebCore/dom/Document.idl b/Source/WebCore/dom/Document.idl
index
2884c1b55ef79605c4085f382094e85c84e4605b..aa30931a24e54826c54d80ff6edad682dd22be70
100644
--- a/Source/WebCore/dom/Document.idl
+++ b/Source/WebCore/dom/Document.idl
@@ -27,7 +27,7 @@
     readonly attribute DOMImplementation implementation;
     readonly attribute Element documentElement;
 
-    [ReturnNewObject, DeliverCustomElementCallbacks, PerWorldBindings,
RaisesException] Element
createElement([TreatNullAs=NullString,Optional=DefaultIsUndefined] DOMString
tagName);
+    [ReturnNewObject, DeliverCustomElementCallbacks, PerWorldBindings,
ActivityLog=AccessForIsolatedWorlds, RaisesException] Element
createElement([TreatNullAs=NullString,Optional=DefaultIsUndefined] DOMString
tagName);
     DocumentFragment   createDocumentFragment();
     [ReturnNewObject, PerWorldBindings] Text
createTextNode([Optional=DefaultIsUndefined] DOMString data);
     [ReturnNewObject] Comment createComment([Optional=DefaultIsUndefined]
DOMString data);
@@ -40,9 +40,9 @@
 
     // Introduced in DOM Level 2:
 
-    [ReturnNewObject, DeliverCustomElementCallbacks, RaisesException] Node
importNode([Optional=DefaultIsUndefined] Node importedNode,
+    [ReturnNewObject, DeliverCustomElementCallbacks, PerWorldBindings,
ActivityLog=AccessForIsolatedWorlds, RaisesException] Node
importNode([Optional=DefaultIsUndefined] Node importedNode,
                     [Optional] boolean deep);
-    [ReturnNewObject, DeliverCustomElementCallbacks, RaisesException] Element
createElementNS([TreatNullAs=NullString,Optional=DefaultIsUndefined] DOMString
namespaceURI,
+    [ReturnNewObject, DeliverCustomElementCallbacks, PerWorldBindings,
ActivityLog=AccessForIsolatedWorlds, RaisesException] Element
createElementNS([TreatNullAs=NullString,Optional=DefaultIsUndefined] DOMString
namespaceURI,
                            
[TreatNullAs=NullString,Optional=DefaultIsUndefined] DOMString qualifiedName);
     [ReturnNewObject, RaisesException] Attr
createAttributeNS([TreatNullAs=NullString,Optional=DefaultIsUndefined] DOMString
namespaceURI,
                                                                          
[TreatNullAs=NullString,Optional=DefaultIsUndefined] DOMString qualifiedName);
@@ -141,7 +141,7 @@
     NodeList getItems([TreatNullAs=NullString, TreatUndefinedAs=NullString,
Optional=DefaultIsUndefined] DOMString typeNames);
 #endif
 
-    [Custom] attribute Location location;
+    [Custom, PerWorldBindings, ActivityLog=AccessForIsolatedWorlds] attribute
Location location;
 
     // IE extensions
 
@@ -285,8 +285,8 @@
 
 #if defined(ENABLE_CUSTOM_ELEMENTS) && ENABLE_CUSTOM_ELEMENTS
     [EnabledAtRuntime=customDOMElements, Conditional=CUSTOM_ELEMENTS,
ImplementedAs=registerElement, CallWith=ScriptState,
DeliverCustomElementCallbacks, RaisesException] CustomElementConstructor
webkitRegister(DOMString name, [Optional] Dictionary options);
-    [ReturnNewObject, DeliverCustomElementCallbacks, PerWorldBindings,
RaisesException] Element createElement(DOMString localName,
[TreatNullAs=NullString] DOMString typeExtension);
-    [ReturnNewObject, DeliverCustomElementCallbacks, RaisesException] Element
createElementNS([TreatNullAs=NullString] DOMString namespaceURI, DOMString
qualifiedName,
+    [ReturnNewObject, DeliverCustomElementCallbacks, PerWorldBindings,
ActivityLog=AccessForIsolatedWorlds, RaisesException] Element
createElement(DOMString localName, [TreatNullAs=NullString] DOMString
typeExtension);
+    [ReturnNewObject, DeliverCustomElementCallbacks, PerWorldBindings,
ActivityLog=AccessForIsolatedWorlds, RaisesException] Element
createElementNS([TreatNullAs=NullString] DOMString namespaceURI, DOMString
qualifiedName,
                             [TreatNullAs=NullString] DOMString typeExtension);
 #endif

[commit-bot: I haz the power, 2013-04-18 21:57:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ulfar@google.com/13799007/49001

[commit-bot: I haz the power, 2013-04-18 21:57:45]

Presubmit check for 13799007-49001 failed and returned exit status 1.

INFO:root:Found 20 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/blink/PRESUBMIT.py

** Presubmit Warnings **
Found lines longer than 250 characters (first 5 shown).
  Source/bindings/tests/results/V8TestObj.cpp, line 4018, 279 chars \
  Source/bindings/tests/results/V8TestObj.cpp, line 4020, 413 chars \
  Source/bindings/tests/results/V8TestObj.cpp, line 4022, 488 chars \
  Source/bindings/tests/results/V8TestObj.cpp, line 4024, 297 chars \
  Source/bindings/tests/results/V8TestObj.cpp, line 4026, 443 chars

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    Source/modules/geolocation/Geolocation.idl
    Source/modules/notifications/DOMWindowNotifications.idl
    Source/modules/notifications/NotificationCenter.idl
    Source/modules/webdatabase/DOMWindowWebDatabase.idl

Presubmit checks took 3.0s to calculate.

[ulfar, 2013-04-18 22:39:05]

Trying to submit.  Can I get an LGTM.

[abarth-chromium, 2013-04-18 23:07:21]

Sorry, we changed the OWNERS rules out from under you.  IDL changes in Modules
LGTM.

[commit-bot: I haz the power, 2013-04-18 23:07:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ulfar@google.com/13799007/52003

[commit-bot: I haz the power, 2013-04-18 23:32:13]

"webkit_tests" failed. Giving up immediately.
Builder is mac_layout_rel, revision is HEAD

[commit-bot: I haz the power, 2013-04-19 01:15:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ulfar@google.com/13799007/52003

[commit-bot: I haz the power, 2013-04-19 01:15:43]

"webkit_tests" failed. Giving up immediately.
Builder is mac_layout_rel, revision is HEAD

[commit-bot: I haz the power, 2013-04-19 01:17:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ulfar@google.com/13799007/52003

[commit-bot: I haz the power, 2013-04-19 01:17:53]

"webkit_tests" failed. Giving up immediately.
Builder is mac_layout_rel, revision is HEAD

[commit-bot: I haz the power, 2013-04-19 20:52:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ulfar@google.com/13799007/52003

[commit-bot: I haz the power, 2013-04-19 20:53:10]

Change committed as 148749