Bug 69266 - REGRESSION (r84225): Virtual function called from Node::parentNode()
Summary: REGRESSION (r84225): Virtual function called from Node::parentNode()
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominic Cooney
Depends on: 69406
  Show dependency treegraph
Reported: 2011-10-03 09:37 PDT by Antti Koivisto
Modified: 2011-10-06 05:52 PDT (History)
7 users (show)

See Also:

Patch (14.62 KB, patch)
2011-10-05 08:35 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 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.
Comment 1 Antti Koivisto 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.
Comment 2 Dominic Cooney 2011-10-03 15:31:37 PDT
OK. Does SVGShadowRoot warrant its own flag to avoid this?
Comment 3 Antti Koivisto 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".
Comment 4 Antti Koivisto 2011-10-03 15:39:04 PDT
We are running out of bits in Node so preferably not.
Comment 5 Dominic Cooney 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.
Comment 6 Dominic Cooney 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.
Comment 7 Roland Steiner 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().
Comment 8 Dominic Cooney 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.
Comment 9 Dominic Cooney 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.
Comment 10 Dominic Cooney 2011-10-05 08:35:36 PDT
Created attachment 109801 [details]

Depends on patch on 69406.
Comment 11 Roland Steiner 2011-10-06 00:29:54 PDT
(In reply to comment #10)

Looks good! :)
Comment 12 WebKit Review Bot 2011-10-06 05:52:48 PDT
Comment on attachment 109801 [details]

Clearing flags on attachment: 109801

Committed r96803: <http://trac.webkit.org/changeset/96803>
Comment 13 WebKit Review Bot 2011-10-06 05:52:53 PDT
All reviewed patches have been landed.  Closing bug.