Bug 69266

Summary: REGRESSION (r84225): Virtual function called from Node::parentNode()
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: DOMAssignee: Dominic Cooney <dominicc>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, koivisto, krit, morrita, rolandsteiner, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 69406    
Bug Blocks:    
Attachments:
Description Flags
Patch none

Antti Koivisto
Reported 2011-10-03 09:37:10 PDT
http://trac.webkit.org/changeset/84225 from bug 52788 added a virtual function call (isSVGShadowRoot()) to Node::parentNode(): inline ContainerNode* Node::parentNode() const { - return getFlag(IsShadowRootFlag) ? 0 : parent(); + return getFlag(IsShadowRootFlag) || isSVGShadowRoot() ? 0 : parent(); } This regresses everything that depends on walking up the tree, most crucially CSS selector matching.
Attachments
Patch (14.62 KB, patch)
2011-10-05 08:35 PDT, Dominic Cooney 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Antti Koivisto
Comment 1 2011-10-03 09:39:18 PDT
The fact that the call is carefully inlined should be a hint that you are not supposed to put a virtual call here.
Dominic Cooney
Comment 2 2011-10-03 15:31:37 PDT
OK. Does SVGShadowRoot warrant its own flag to avoid this?
Antti Koivisto
Comment 3 2011-10-03 15:37:26 PDT
Why does it need a separate test? It makes no logical sense that a "SVG shadow root" is not a "shadow root".
Antti Koivisto
Comment 4 2011-10-03 15:39:04 PDT
We are running out of bits in Node so preferably not.
Dominic Cooney
Comment 5 2011-10-03 16:14:25 PDT
Originally there was an attempt to merge ShadowRoot and SVGShadowRoot, but it turned out to be difficult—dglazkov should be able to provide context. I attempted to separate them which is what motivated this change.
Dominic Cooney
Comment 6 2011-10-03 16:27:59 PDT
To add some commentary on what was difficult about merging ShadowRoot and SVGShadowRoot—IIRC: - Rendering is different. - SVGShadowRoot tries to behave like <svg:g>, which is not helpful when ShadowRoot appears in a HTML tree. - Event dispatch is different. - There’s a handful of special behavior for SVGShadowRoot, eg can’t have <script> descendants. - There’s a handful of special behavior for ShadowRoot around editing. CC’d Krit in case he is working on any refactoring around SVGShadowRoot.
Roland Steiner
Comment 7 2011-10-03 17:10:44 PDT
I think that sums it up pretty nicely. One other approach we were discussing is to set the parent pointer to NULL, the host node holding an explicit ref on the (SVG) shadow root instead, and having a separate host element pointer on the (SVG) shadow root. However, until we add ShadowRoot to SVG, this merely pushes the virtual function call from parentNode() to parentOrHostNode().
Dominic Cooney
Comment 8 2011-10-03 18:15:33 PDT
Maybe right now we can move back to using the 'shadow root' flag for ShadowRoot and SVGShadowRoot. This will make parentNode() fast. Some editing code may slow down, although maybe we can make isShadowRoot do getFlag(ShadowRoot) && !n->isSVGShadowRoot() which is still fast in <input>, <textarea>, etc. when the result is false.
Dominic Cooney
Comment 9 2011-10-04 21:06:35 PDT
Another awkward difference between ShadowRoot and SVGShadowRoot is ShadowRoot sets and clears the flag when it goes into and out of the tree, but isSVGShadowRoot is constant. A big hacky, but parentNode can be trivially sped up in the non-SVG case with something like: return getFlag(IsShadowRootFlag) || (isSVGElement() && isSVGShadowRoot()) ? 0 : parent(); which predicates the virtual call on another flag test. For now I am going to look at using these pairs of flags: IsShadowRootFlag, not IsSVGElementFlag => ShadowRoot IsShadowRootFlag, IsSVGElementFlag => SVGShadowRoot I will rename the schizoflag to IsShadowRootOrSVGShadowRootFlag.
Dominic Cooney
Comment 10 2011-10-05 08:35:36 PDT
Created attachment 109801 [details] Patch Depends on patch on 69406.
Roland Steiner
Comment 11 2011-10-06 00:29:54 PDT
(In reply to comment #10) Looks good! :)
WebKit Review Bot
Comment 12 2011-10-06 05:52:48 PDT
Comment on attachment 109801 [details] Patch Clearing flags on attachment: 109801 Committed r96803: <http://trac.webkit.org/changeset/96803>
WebKit Review Bot
Comment 13 2011-10-06 05:52:53 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.