Summary: | [V8] Use v8::V8::AddImplicitReferences instead of SetHiddenValue | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Erik Arvidsson <arv> | ||||||||||||||||||||
Component: | DOM | Assignee: | Erik Arvidsson <arv> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | abarth, adamk, antonm, cmarcelo, dglazkov, eric.carlson, feature-media-reviews, haraken, japhet, jochen, kbr, macpherson, menard, morrita, ojan, rakuco, webkit.review.bot | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | 80936, 81170, 89604 | ||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||
Attachments: |
|
Description
Erik Arvidsson
2012-03-12 13:48:15 PDT
Created attachment 131404 [details]
Patch
Comment on attachment 131404 [details] Patch Attachment 131404 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11943156 Comment on attachment 131404 [details]
Patch
This patch looks really cool. It looks like you have a compilation issue though.
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. ;)
(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. Wonderful patch! Created attachment 131448 [details]
Patch for landing
Comment on attachment 131448 [details] Patch for landing Clearing flags on attachment: 131448 Committed r110524: <http://trac.webkit.org/changeset/110524> All reviewed patches have been landed. Closing bug. Rolled out at http://trac.webkit.org/changeset/110537 because it broke chromium-win build. (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. The patch also seemed to cause a number of ASSERTs. 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 = ...) { (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? Created attachment 131692 [details]
Patch
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. Comment on attachment 131692 [details]
Patch
I'll see if I can find those asserts (this is the first time I hear about them)
If you have your IRC log from last night, I sent you a link to them. 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 Got it. Thanks. Created attachment 131716 [details]
Patch
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. Comment on attachment 131716 [details]
Patch
Ok.
Comment on attachment 131716 [details] Patch Clearing flags on attachment: 131716 Committed r110641: <http://trac.webkit.org/changeset/110641> All reviewed patches have been landed. Closing bug. (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. Is the landed patch correct? Now I can find no "visitDOMWrapper" in the generated code. 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). 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? Ok Created attachment 148622 [details]
Patch for landing
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 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.
(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). Created attachment 148627 [details]
Patch for landing
Comment on attachment 148627 [details] Patch for landing Clearing flags on attachment: 148627 Committed r120854: <http://trac.webkit.org/changeset/120854> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by 89604 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 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 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: # 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. Created attachment 148683 [details]
Patch
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. 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. Created attachment 148860 [details]
Patch for landing
Comment on attachment 148860 [details] Patch for landing Clearing flags on attachment: 148860 Committed r120968: <http://trac.webkit.org/changeset/120968> All reviewed patches have been landed. Closing bug. |