See http://code.google.com/p/chromium/issues/detail?id=64695 the problem is simple and alike to the problem of implicit references between nodes in document tree. The solution is to group all the style elements which are reachable via DOM API into v8's object group. I don't know details of style elements reachability, so I do the same thing we do for nodes: I group all the style elements which belong to the same tree as determined by the root element of the tree.
Created attachment 75165 [details] Group all style elements in v8 object group
Created attachment 75174 [details] [minor] proper diff Proper diff---the previous one fail to apply
Attachment 75174 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/bindings/v8/V8DOMMap.h', u'WebCore/bindings/v8/V8GCController.cpp']" exit_code: 1 WebCore/bindings/v8/V8GCController.cpp:45: Alphabetical sorting problem. [build/include_order] [4] WebCore/bindings/v8/V8GCController.cpp:50: Alphabetical sorting problem. [build/include_order] [4] WebCore/bindings/v8/V8GCController.cpp:358: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/bindings/v8/V8GCController.cpp:362: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 4 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 75175 [details] Fixing style issues
Attachment 75175 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/bindings/v8/V8DOMMap.h', u'WebCore/bindings/v8/V8GCController.cpp']" exit_code: 1 WebCore/bindings/v8/V8GCController.cpp:49: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 75178 [details] Next round of style fixes
Does this also fix (or could it be extended slightly to fix) http://trac.webkit.org/changeset/72344 and http://trac.webkit.org/changeset/72590 ? Also, it seems like this should remove the corresponding lines in test_expectations: BUG_LOISLO : fast/dom/StyleSheet/gc-rule-children-wrappers.html = TEXT BUG_LEVIN : fast/dom/StyleSheet/gc-inline-style-cssvalues.html = TEXT BUG_LEVIN : fast/dom/StyleSheet/gc-styleheet-wrapper.xhtml = TEXT BUGCR64695 : fast/dom/StyleSheet/gc-parent-rule.html = TEXT BUGCR64695 : fast/dom/StyleSheet/gc-parent-stylesheet.html = TEXT BUGCR64695 : fast/dom/StyleSheet/gc-declaration-parent-rule.html = TEXT Trivial nit: ObjectGrouperVisitor::startMap (endMap) need 4 space indenting. cc'ed a few folks whom I believe know v8 well enough to review this effectively.
Added someone else who may find it interesting (since they also added a line in test expectations that could be fixed by this).
Created attachment 75291 [details] Another style nit
(In reply to comment #7) > Does this also fix (or could it be extended slightly to fix) > http://trac.webkit.org/changeset/72344 > and > http://trac.webkit.org/changeset/72590 > ? > > Also, it seems like this should remove the corresponding lines in test_expectations: > BUG_LOISLO : fast/dom/StyleSheet/gc-rule-children-wrappers.html = TEXT > BUG_LEVIN : fast/dom/StyleSheet/gc-inline-style-cssvalues.html = TEXT > BUG_LEVIN : fast/dom/StyleSheet/gc-styleheet-wrapper.xhtml = TEXT > BUGCR64695 : fast/dom/StyleSheet/gc-parent-rule.html = TEXT > BUGCR64695 : fast/dom/StyleSheet/gc-parent-stylesheet.html = TEXT > BUGCR64695 : fast/dom/StyleSheet/gc-declaration-parent-rule.html = TEXT Chances are high. It's not quite convenient for me to run X apps (like test_shell) this week as I am traveling. I can do it myself next week and clean unnecessary suppressions. I'll try to fix reminding problems too. > > Trivial nit: ObjectGrouperVisitor::startMap (endMap) need 4 space indenting. Thanks a lot, fixed. > > cc'ed a few folks whom I believe know v8 well enough to review this effectively. Ojan is going to run my patch through layout tests and see if it's good. BTW, I'd esp. appreciate a look from people who are good in WebKit CSS model---we need to ensure that all implicitly style elements are put into v8's object group. Thanks a lot for your help, David.
I did run the test through layout tests. It fixed fast/dom/StyleSheet/gc-parent-rule.html and fast/dom/StyleSheet/gc-parent-stylesheet.html, but not fast/dom/StyleSheet/gc-declaration-parent-rule.html. I'd prefer someone more familiar with V8 bindings review this though.
Comment on attachment 75291 [details] Another style nit View in context: https://bugs.webkit.org/attachment.cgi?id=75291&action=review I'm not sure I understand this part of the V8 bindings well enough to give it a thorough review, but I'll take a stab at it... > WebCore/bindings/v8/V8GCController.cpp:330 > + /* FIXME: Re-enabled this code to avoid GCing these wrappers! > + Currently this depends on looking up the wrapper > + during a GC, but we don't know which isolated world > + we're in, so it's unclear which map to look in... Are this FIXME and commented-out block still valid? I'm just confused because it appears they didn't move with the surrounding code. > WebCore/bindings/v8/V8GCController.cpp:357 > +class ObjectGrouperVisitor : public DOMWrapperMap<void>::Visitor { > +public: > + ObjectGrouperVisitor() > + { > + } > This name seems too generic for what this class is doing (or am I misunderstanding?). Perhaps StyleBaseGrouperVisitor? > WebCore/bindings/v8/V8GCController.cpp:378 > + // FIXME: extend WrapperTypeInfo with isStyle to simplify the check below. > + // FIXME: check if there are other StyleBase wrappers we should care of. > + if (!V8CSSStyleSheet::info.equals(typeInfo) > + && !V8CSSCharsetRule::info.equals(typeInfo) > + && !V8CSSFontFaceRule::info.equals(typeInfo) > + && !V8CSSStyleRule::info.equals(typeInfo) > + && !V8CSSImportRule::info.equals(typeInfo) > + && !V8CSSMediaRule::info.equals(typeInfo)) { A simplification is definitely desirable, though I'm fine if it's in a separate patch. Unfortunately, StyleBase doesn't have an idl, so we'd either need to cheat with an idl attribute (or dummy idl, i suppose), or maintain an Evil List in CodeGeneratorV8.pm, a la ActiveDOMObject. > WebCore/bindings/v8/V8GCController.cpp:423 > ObjectGrouperVisitor objectGrouperVisitor; > - visitDOMNodesInCurrentThread(&objectGrouperVisitor); > - objectGrouperVisitor.applyGrouping(); > + visitDOMObjectsInCurrentThread(&objectGrouperVisitor); This call to visitDOMObjectsInCurrentThread() looks like it might now be equivalent to the NDEBUG block a few lines above. Is it?
Thanks a lot for comments, Nate! (In reply to comment #12) > (From update of attachment 75291 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75291&action=review > > I'm not sure I understand this part of the V8 bindings well enough to give it a thorough review, but I'll take a stab at it... > > > WebCore/bindings/v8/V8GCController.cpp:330 > > + /* FIXME: Re-enabled this code to avoid GCing these wrappers! > > + Currently this depends on looking up the wrapper > > + during a GC, but we don't know which isolated world > > + we're in, so it's unclear which map to look in... > > Are this FIXME and commented-out block still valid? I'm just confused because it appears they didn't move with the surrounding code. Kind of. The additional grouping in the comments should be applied when traversing DOM nodes (I am planning to fix it with the next commit), but it's not generic and, for example, is not applicable for style traversal (at least I think so). > > WebCore/bindings/v8/V8GCController.cpp:357 > > +class ObjectGrouperVisitor : public DOMWrapperMap<void>::Visitor { > > +public: > > + ObjectGrouperVisitor() > > + { > > + } > > > > This name seems too generic for what this class is doing (or am I misunderstanding?). Perhaps StyleBaseGrouperVisitor? Let me explain the reason I named it this way, and if you disagree, I'll rename. I wanted to capture the idea of grouping of the DOM objects (vs. DOM nodes). Currently it groups only styles, but if there is a need to group some other objects, the code could naturally go in there, hence generic "object" as opposite to "node". Maybe something referencing DOMObject would be better. > > WebCore/bindings/v8/V8GCController.cpp:378 > > + // FIXME: extend WrapperTypeInfo with isStyle to simplify the check below. > > + // FIXME: check if there are other StyleBase wrappers we should care of. > > + if (!V8CSSStyleSheet::info.equals(typeInfo) > > + && !V8CSSCharsetRule::info.equals(typeInfo) > > + && !V8CSSFontFaceRule::info.equals(typeInfo) > > + && !V8CSSStyleRule::info.equals(typeInfo) > > + && !V8CSSImportRule::info.equals(typeInfo) > > + && !V8CSSMediaRule::info.equals(typeInfo)) { > > A simplification is definitely desirable, though I'm fine if it's in a separate patch. Unfortunately, StyleBase doesn't have an idl, so we'd either need to cheat with an idl attribute (or dummy idl, i suppose), or maintain an Evil List in CodeGeneratorV8.pm, a la ActiveDOMObject. Thanks, I'd prefer to postpone it then. > > WebCore/bindings/v8/V8GCController.cpp:423 > > ObjectGrouperVisitor objectGrouperVisitor; > > - visitDOMNodesInCurrentThread(&objectGrouperVisitor); > > - objectGrouperVisitor.applyGrouping(); > > + visitDOMObjectsInCurrentThread(&objectGrouperVisitor); > > This call to visitDOMObjectsInCurrentThread() looks like it might now be equivalent to the NDEBUG block a few lines above. Is it? Sorry, I am not sure I understand you: we use different visitors here. What am I missing?
(In reply to comment #13) > Thanks a lot for comments, Nate! > > (In reply to comment #12) > > (From update of attachment 75291 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=75291&action=review > > > > I'm not sure I understand this part of the V8 bindings well enough to give it a thorough review, but I'll take a stab at it... > > > > > WebCore/bindings/v8/V8GCController.cpp:330 > > > + /* FIXME: Re-enabled this code to avoid GCing these wrappers! > > > + Currently this depends on looking up the wrapper > > > + during a GC, but we don't know which isolated world > > > + we're in, so it's unclear which map to look in... > > > > Are this FIXME and commented-out block still valid? I'm just confused because it appears they didn't move with the surrounding code. > > Kind of. The additional grouping in the comments should be applied when traversing DOM nodes (I am planning to fix it with the next commit), but it's not generic and, for example, is not applicable for style traversal (at least I think so). Ok, I just wanted to make sure it hadn't been forgotten. > > > > WebCore/bindings/v8/V8GCController.cpp:357 > > > +class ObjectGrouperVisitor : public DOMWrapperMap<void>::Visitor { > > > +public: > > > + ObjectGrouperVisitor() > > > + { > > > + } > > > > > > > This name seems too generic for what this class is doing (or am I misunderstanding?). Perhaps StyleBaseGrouperVisitor? > > Let me explain the reason I named it this way, and if you disagree, I'll rename. I wanted to capture the idea of grouping of the DOM objects (vs. DOM nodes). Currently it groups only styles, but if there is a need to group some other objects, the code could naturally go in there, hence generic "object" as opposite to "node". Maybe something referencing DOMObject would be better. That seems reasonable to me (I was assuming that we wouldn't be grouping other types of objects in this way, but that's a silly assumption). Perhaps DOMObjectGrouperVisitor then? > > > > WebCore/bindings/v8/V8GCController.cpp:378 > > > + // FIXME: extend WrapperTypeInfo with isStyle to simplify the check below. > > > + // FIXME: check if there are other StyleBase wrappers we should care of. > > > + if (!V8CSSStyleSheet::info.equals(typeInfo) > > > + && !V8CSSCharsetRule::info.equals(typeInfo) > > > + && !V8CSSFontFaceRule::info.equals(typeInfo) > > > + && !V8CSSStyleRule::info.equals(typeInfo) > > > + && !V8CSSImportRule::info.equals(typeInfo) > > > + && !V8CSSMediaRule::info.equals(typeInfo)) { > > > > A simplification is definitely desirable, though I'm fine if it's in a separate patch. Unfortunately, StyleBase doesn't have an idl, so we'd either need to cheat with an idl attribute (or dummy idl, i suppose), or maintain an Evil List in CodeGeneratorV8.pm, a la ActiveDOMObject. > > Thanks, I'd prefer to postpone it then. > > > > WebCore/bindings/v8/V8GCController.cpp:423 > > > ObjectGrouperVisitor objectGrouperVisitor; > > > - visitDOMNodesInCurrentThread(&objectGrouperVisitor); > > > - objectGrouperVisitor.applyGrouping(); > > > + visitDOMObjectsInCurrentThread(&objectGrouperVisitor); > > > > This call to visitDOMObjectsInCurrentThread() looks like it might now be equivalent to the NDEBUG block a few lines above. Is it? > > Sorry, I am not sure I understand you: we use different visitors here. What am I missing? Ah, yes, I misread the code in the NDEBUG block, they are different visitor types. Sorry about that.
Created attachment 75531 [details] Addressing Nate's comments
Thanks for another round, Nate. I've ended up renaming to DOMObjectGrouperVisitor (yummy). May you have another look, please? (In reply to comment #14) > (In reply to comment #13) > > Thanks a lot for comments, Nate! > > > > (In reply to comment #12) > > > (From update of attachment 75291 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=75291&action=review > > > > > > I'm not sure I understand this part of the V8 bindings well enough to give it a thorough review, but I'll take a stab at it... > > > > > > > WebCore/bindings/v8/V8GCController.cpp:330 > > > > + /* FIXME: Re-enabled this code to avoid GCing these wrappers! > > > > + Currently this depends on looking up the wrapper > > > > + during a GC, but we don't know which isolated world > > > > + we're in, so it's unclear which map to look in... > > > > > > Are this FIXME and commented-out block still valid? I'm just confused because it appears they didn't move with the surrounding code. > > > > Kind of. The additional grouping in the comments should be applied when traversing DOM nodes (I am planning to fix it with the next commit), but it's not generic and, for example, is not applicable for style traversal (at least I think so). > > Ok, I just wanted to make sure it hadn't been forgotten. > > > > > > > WebCore/bindings/v8/V8GCController.cpp:357 > > > > +class ObjectGrouperVisitor : public DOMWrapperMap<void>::Visitor { > > > > +public: > > > > + ObjectGrouperVisitor() > > > > + { > > > > + } > > > > > > > > > > This name seems too generic for what this class is doing (or am I misunderstanding?). Perhaps StyleBaseGrouperVisitor? > > > > Let me explain the reason I named it this way, and if you disagree, I'll rename. I wanted to capture the idea of grouping of the DOM objects (vs. DOM nodes). Currently it groups only styles, but if there is a need to group some other objects, the code could naturally go in there, hence generic "object" as opposite to "node". Maybe something referencing DOMObject would be better. > > That seems reasonable to me (I was assuming that we wouldn't be grouping other types of objects in this way, but that's a silly assumption). Perhaps DOMObjectGrouperVisitor then? > > > > > > > WebCore/bindings/v8/V8GCController.cpp:378 > > > > + // FIXME: extend WrapperTypeInfo with isStyle to simplify the check below. > > > > + // FIXME: check if there are other StyleBase wrappers we should care of. > > > > + if (!V8CSSStyleSheet::info.equals(typeInfo) > > > > + && !V8CSSCharsetRule::info.equals(typeInfo) > > > > + && !V8CSSFontFaceRule::info.equals(typeInfo) > > > > + && !V8CSSStyleRule::info.equals(typeInfo) > > > > + && !V8CSSImportRule::info.equals(typeInfo) > > > > + && !V8CSSMediaRule::info.equals(typeInfo)) { > > > > > > A simplification is definitely desirable, though I'm fine if it's in a separate patch. Unfortunately, StyleBase doesn't have an idl, so we'd either need to cheat with an idl attribute (or dummy idl, i suppose), or maintain an Evil List in CodeGeneratorV8.pm, a la ActiveDOMObject. > > > > Thanks, I'd prefer to postpone it then. > > > > > > WebCore/bindings/v8/V8GCController.cpp:423 > > > > ObjectGrouperVisitor objectGrouperVisitor; > > > > - visitDOMNodesInCurrentThread(&objectGrouperVisitor); > > > > - objectGrouperVisitor.applyGrouping(); > > > > + visitDOMObjectsInCurrentThread(&objectGrouperVisitor); > > > > > > This call to visitDOMObjectsInCurrentThread() looks like it might now be equivalent to the NDEBUG block a few lines above. Is it? > > > > Sorry, I am not sure I understand you: we use different visitors here. What am I missing? > > Ah, yes, I misread the code in the NDEBUG block, they are different visitor types. Sorry about that.
Created attachment 75534 [details] WebKit hacking from Chromium checkout over ssh sucks
Comment on attachment 75534 [details] WebKit hacking from Chromium checkout over ssh sucks View in context: https://bugs.webkit.org/attachment.cgi?id=75534&action=review > WebCore/ChangeLog:22 > + (WebCore::ObjectGrouperVisitor::ObjectGrouperVisitor): > + (WebCore::ObjectGrouperVisitor::startMap): > + (WebCore::ObjectGrouperVisitor::endMap): > + (WebCore::ObjectGrouperVisitor::visitDOMWrapper): Nit: this didn't get changed to DOMObjectGrouperVisitor. > WebCore/bindings/v8/V8GCController.cpp:385 > + StyleBase* styleBase = static_cast<StyleBase*>(object); > > - ASSERT(i == nextKeyIndex); > + // We put the whole tree of style elements into a single object group. > + // To achieve that we group elements by the roots of their trees. > + StyleBase* root = styleBase; I wish there were a way for this code to be clear without needing styleBase as a separate variable (it's only use is to initialize root), but I don't immediately see what it is.
Created attachment 75804 [details] Last minor fixes
Thanks a lot, Nate. Fixing and cq+'ing (In reply to comment #18) > (From update of attachment 75534 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75534&action=review > > > WebCore/ChangeLog:22 > > + (WebCore::ObjectGrouperVisitor::ObjectGrouperVisitor): > > + (WebCore::ObjectGrouperVisitor::startMap): > > + (WebCore::ObjectGrouperVisitor::endMap): > > + (WebCore::ObjectGrouperVisitor::visitDOMWrapper): > > Nit: this didn't get changed to DOMObjectGrouperVisitor. > > > WebCore/bindings/v8/V8GCController.cpp:385 > > + StyleBase* styleBase = static_cast<StyleBase*>(object); > > > > - ASSERT(i == nextKeyIndex); > > + // We put the whole tree of style elements into a single object group. > > + // To achieve that we group elements by the roots of their trees. > > + StyleBase* root = styleBase; > > I wish there were a way for this code to be clear without needing styleBase as a separate variable (it's only use is to initialize root), but I don't immediately see what it is.
Created attachment 75805 [details] With proper reviewed by
Comment on attachment 75805 [details] With proper reviewed by Clearing flags on attachment: 75805 Committed r73491: <http://trac.webkit.org/changeset/73491>
All reviewed patches have been landed. Closing bug.