Bug 80880 – [V8] Use v8::V8::AddImplicitReferences instead of SetHiddenValue

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Erik Arvidsson]

Created attachment 131400 [details]
Per test

This is part of the patch from bug 78149 but without the collection part. I'm doing this in two parts since using v8::V8::AddHiddenReferences is an improvement over SetHiddenValue by itself.

In the attached perf test creating a classList is 3.4 times faster.

With patch

1.46ns/classList
1.41ns/classList
1.47ns/classList
1.41ns/classList

Without patch

4.890000000000001ns/classList
4.8999999999999995ns/classList
4.83ns/classList
4.9399999999999995ns/classList

[Erik Arvidsson]

Created attachment 131404 [details]
Patch

[WebKit Review Bot]

Comment on attachment 131404 [details]
Patch

Attachment 131404 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11943156

[Adam Barth]

Comment on attachment 131404 [details]
Patch

This patch looks really cool.  It looks like you have a compilation issue though.

[Adam Barth]

Comment on attachment 131404 [details]
Patch

I'm marking this r+ out of pure enthusiasm, but we'll probably need to fix the compile errors before landing.  ;)

[Erik Arvidsson]

(In reply to comment #3)
> (From update of attachment 131404 [details])
> This patch looks really cool.  It looks like you have a compilation issue though.

I got the compile error under control. I was dissecting the initial patch which also set up hidden references for collections.

[Kentaro Hara]

Wonderful patch!

[Erik Arvidsson]

Created attachment 131448 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 131448 [details]
Patch for landing

Clearing flags on attachment: 131448

Committed r110524: <http://trac.webkit.org/changeset/110524>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hajime Morrita]

Rolled out at http://trac.webkit.org/changeset/110537 because it broke chromium-win build.

[Hajime Morrita]

(In reply to comment #10)
> Rolled out at http://trac.webkit.org/changeset/110537 because it broke chromium-win build.
FYI: http://chromegw.corp.google.com/i/chromium.webkit/builders/Webkit%20Win%20Builder%20%28dbg%29/builds/21087/steps/compile/logs/stdio

It looks that the generated source file doesn't include HTMLMediaElement.h and
cannot resolve HTMLMediaElement as a Node subclass.

[Adam Barth]

The patch also seemed to cause a number of ASSERTs.

[anton muhin]

Comment on attachment 131448 [details]
Patch for landing

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

LGTM, pretty neat

> Source/WebCore/ChangeLog:3
> +        [V8] Use v8::V8::AddHiddenReferences instead of SetHiddenValue

Add<Implicit>References?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2317
> +            } elsif (GetGenerateIsReachable($dataNode) eq "ImplFrame") {

up to you, but looks slightly scary: you'll now return value for GenerateIsReachable, not JSGenerateIsReachable.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:237
> +    if (owner) {

nit: if (Node* owner = ...) {

[Erik Arvidsson]

(In reply to comment #13)
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2317
> > +            } elsif (GetGenerateIsReachable($dataNode) eq "ImplFrame") {
> 
> up to you, but looks slightly scary: you'll now return value for GenerateIsReachable, not JSGenerateIsReachable.

The idea is that GenerateReachable = JSGenerateIsReachable | V8GenerateIsReachable. This pattern is used all over in the bindings.

Maybe I am missing something?

[Erik Arvidsson]

Created attachment 131692 [details]
Patch

[Adam Barth]

Comment on attachment 131692 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        Second try. This time with an added include in TextTrackList.cpp.

Did you change more than just adding the include?  There were a bunch of debug crashes (ASSERTSs?) triggered by this change the first time.

[Erik Arvidsson]

Comment on attachment 131692 [details]
Patch

I'll see if I can find those asserts (this is the first time I hear about them)

[Adam Barth]

If you have your IRC log from last night, I sent you a link to them.

[Adam Barth]

I found a link in my IRC log:
http://build.chromium.org/f/chromium/layout_test_results/Webkit_Linux__dbg_/results/layout-test-results/dom/html/level1/core/hc_attrgetvalue1-crash-log.txt

[Erik Arvidsson]

Got it. Thanks.

[Erik Arvidsson]

Created attachment 131716 [details]
Patch

[Erik Arvidsson]

It turns out that the interfaces with GenerateReachable also needs V8DependentLifeTime since these are not independent. I updated the codegen to set the correct hasDependentLifetime.

[Adam Barth]

Comment on attachment 131716 [details]
Patch

Ok.

[WebKit Review Bot]

Comment on attachment 131716 [details]
Patch

Clearing flags on attachment: 131716

Committed r110641: <http://trac.webkit.org/changeset/110641>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[anton muhin]

(In reply to comment #14)
> (In reply to comment #13)
> > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2317
> > > +            } elsif (GetGenerateIsReachable($dataNode) eq "ImplFrame") {
> > 
> > up to you, but looks slightly scary: you'll now return value for GenerateIsReachable, not JSGenerateIsReachable.
> 
> The idea is that GenerateReachable = JSGenerateIsReachable | V8GenerateIsReachable. This pattern is used all over in the bindings.
> 
> Maybe I am missing something?

No, I don't think so.  Thanks a lot for explanations.

[Kentaro Hara]

Is the landed patch correct? Now I can find no "visitDOMWrapper" in the generated code.

[Adam Barth]

Sorry, this got reverted again because it had ASAN problems:


AllUrlsApiTest.WhitelistedExtension: 
[6008:6008:0314/155218:1809070530:WARNING:zygote_host_impl_linux.cc(154)] Running without the SUID sandbox! See http://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment for more information on developing with the sandbox on.
[6008:6008:0314/155219:1810342920:WARNING:CONSOLE(2109)] "Missing string for id: login_status_message", source: chrome://newtab/ (2109)
[6008:6008:0314/155219:1810343124:WARNING:CONSOLE(2109)] "Missing string for id: ntp4_intro_message", source: chrome://newtab/ (2109)
[6008:6008:0314/155219:1810343207:WARNING:CONSOLE(2109)] "Missing string for id: serverpromo", source: chrome://newtab/ (2109)
ASAN:SIGSEGV
==6041== ERROR: AddressSanitizer crashed on unknown address 0x000000000000 (pc 0x00000396c3e9 sp 0x7fffeab329f0 bp 0x7fffeab32a20 T0)
AddressSanitizer can not provide additional info. ABORTING
    #0 0x396c3e9 in v8::Context::Global() ???:0
    #1 0x5a4e8ec in WebCore::DOMData::getCurrentStore() ???:0
    #2 0x52242d9 in WebCore::getDOMNodeMap() ???:0
    #3 0x659e806 in WebCore::V8DOMTokenList::visitDOMWrapper(WebCore::DOMDataStore*, void*, v8::Persistent<v8::Object>) ???:0
    #4 0x5a5634f in WebCore::WeakReferenceMap<void, v8::Object>::visit(WebCore::DOMDataStore*, WebCore::AbstractWeakReferenceMap<void, v8::Object>::Visitor*) ???:0
    #5 0x5224eb8 in WebCore::visitDOMObjects(WebCore::AbstractWeakReferenceMap<void, v8::Object>::Visitor*) ???:0
    #6 0x522e4b6 in WebCore::V8GCController::gcPrologue() ???:0
    #7 0x3a6c83e in v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::internal::GCTracer*) ???:0
Stats: 35M malloced (47M for red zones) by 142313 calls
Stats: 0M realloced by 258 calls
Stats: 29M freed by 81525 calls
Stats: 0M really freed by 0 calls
Stats: 112M (28687 full pages) mmaped in 28 calls
  mmaps   by size class: 8:131064; 9:16382; 10:8190; 11:2047; 12:1024; 13:1024; 14:512; 15:128; 16:128; 17:96; 18:32; 19:8; 20:4;
  mallocs by size class: 8:124488; 9:9231; 10:6162; 11:801; 12:317; 13:680; 14:334; 15:115; 16:72; 17:79; 18:26; 19:7; 20:1;
  frees   by size class: 8:67711; 9:6245; 10:5667; 11:547; 12:251; 13:546; 14:291; 15:103; 16:60; 17:70; 18:26; 19:7; 20:1;
  rfrees  by size class:
Stats: malloc large: 113 small slow: 511
Killed (timed out).

[Erik Arvidsson]

Adam (Barth), I cannot reproduce this ASAN report and Adam (Klein) suspects it is a ASAN bug. My plan is to try to submit this again Wed next week and see if it still happens. Are you OK with that?

[Adam Barth]

Ok

[Erik Arvidsson]

Created attachment 148622 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 148622 [details]
Patch for landing

Rejecting attachment 148622 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13010011

[Erik Arvidsson]

Comment on attachment 148622 [details]
Patch for landing

This has no changes since last time it was r+ed. The only diffs are due to rebaseling.

[Adam Klein]

(In reply to comment #33)
> (From update of attachment 148622 [details])
> This has no changes since last time it was r+ed. The only diffs are due to rebaseling.

In that case you can just put the reviewer in the ChangeLog(s) and cq+ it (which is all webkit-patch land-safely does anyway).

[Erik Arvidsson]

Created attachment 148627 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 148627 [details]
Patch for landing

Clearing flags on attachment: 148627

Committed r120854: <http://trac.webkit.org/changeset/120854>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 89604

[Erik Arvidsson]

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20%28dbg%29/builds/8243

Regressions: Unexpected crashes : (2)
  inspector/extensions/extensions-audits-content-script.html = CRASH
  inspector/extensions/extensions-eval-content-script.html = CRASH

[Kenneth Russell]

Crash was observed in release build as well:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/27170

Regressions: Unexpected crashes : (1)
  inspector/extensions/extensions-audits-content-script.html = CRASH

14:55:27.236 7483 worker/3 inspector/extensions/extensions-audits-content-script.html crashed, stack trace:
14:55:27.236 7483   	base::debug::StackTrace::StackTrace() [0x673c5e]
14:55:27.236 7483   	base::(anonymous namespace)::StackDumpSignalHandler() [0x6602b9]
14:55:27.236 7483   	0x7f5a4d43eaf0
14:55:27.236 7483   	WebCore::DOMData::getCurrentStore() [0xfd35ab]
14:55:27.236 7483   	WebCore::getDOMNodeMap() [0xce0639]
14:55:27.236 7483   	WebCore::V8DOMTokenList::visitDOMWrapper() [0x137bcb9]
14:55:27.236 7483   	WebCore::WeakReferenceMap<>::visit() [0xfd5bf6]
14:55:27.236 7483   	WebCore::visitDOMObjects() [0xce04c9]
14:55:27.236 7483   	WebCore::V8GCController::gcPrologue() [0xce5de3]
14:55:27.236 7483   	v8::internal::Heap::PerformGarbageCollection() [0x8126bf]
14:55:27.236 7483   	v8::internal::Heap::CollectGarbage() [0x81301f]
14:55:27.236 7483   	v8::internal::Heap::CollectAllGarbage() [0x813520]
14:55:27.236 7483   	v8::internal::Debug::GetLoadedScripts() [0x7bc544]
14:55:27.236 7483   	v8::internal::Runtime_DebugGetLoadedScripts() [0x933cce]
14:55:27.236 7483   	0x1bd65730618e
14:55:27.238 7462   inspector/extensions/extensions-audits-content-script.html -> unexpected crash

[Adam Klein]

From the debug build, an assertion in V8:

STDERR: #
STDERR: # Fatal error in v8/src/objects-inl.h, line 1489
STDERR: # CHECK(index < GetInternalFieldCount() && index >= 0) failed
STDERR: #

[Erik Arvidsson]

Comment on attachment 148627 [details]
Patch for landing

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

Adam Klein helped me figure this out. A new patch is coming.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:226
> +        v8::Persistent<v8::Object> ownerWrapper = getDOMNodeMap().get(owner);

This line was the problem. It needs to be store->domNodeMap() to get the correct map.

[Erik Arvidsson]

Created attachment 148683 [details]
Patch

[anton muhin]

Comment on attachment 148683 [details]
Patch

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

neat

> Source/WebCore/bindings/v8/V8GCController.cpp:344
> +        if (info->domWrapperVisitorFunction)

I think it's time to make WrapperTypeInfo a proper class and switch to virtual functions.

[Erik Arvidsson]

Comment on attachment 148683 [details]
Patch

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

>> Source/WebCore/bindings/v8/V8GCController.cpp:344
>> +        if (info->domWrapperVisitorFunction)
> 
> I think it's time to make WrapperTypeInfo a proper class and switch to virtual functions.

That seems lika a good idea. I can do that in a follow up patch.

[Erik Arvidsson]

Created attachment 148860 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 148860 [details]
Patch for landing

Clearing flags on attachment: 148860

Committed r120968: <http://trac.webkit.org/changeset/120968>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.