|Summary:||[Refactoring] Accessing Node::m_document should be minimized.|
|Product:||WebKit||Reporter:||Hajime Morrita <morrita>|
|Component:||DOM||Assignee:||Hajime Morrita <morrita>|
|Severity:||Normal||CC:||ap, cdumez, dglazkov, dominicc, husky, ossy, rolandsteiner, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:||73827|
Description Hajime Morrita 2011-12-04 22:54:33 PST
It should be indirected by Node::document() because Node::document() is going to be a bit nontrivial.
Comment 2 Kent Tamura 2011-12-05 01:20:42 PST
Comment on attachment 117851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117851&action=review > Source/WebCore/ChangeLog:12 > + [Refactoring] Accessing Node::m_document should be minimized. > + https://bugs.webkit.org/show_bug.cgi?id=73800 > + > + Reviewed by NOBODY (OOPS!). > + > + No new tests. No behavioral change. > + > + Replaced m_document reference with the document() accessor > + or temporaril variables. > + Please write a reason of the change.
Comment 3 WebKit Review Bot 2011-12-05 01:27:10 PST
Comment on attachment 117851 [details] Patch Attachment 117851 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10688615 New failing tests: svg/custom/linking-uri-01-b.svg
Comment 4 Hajime Morrita 2011-12-05 03:00:36 PST
(In reply to comment #2) > > + > > Please write a reason of the change. Hi Kent-san, thanks for quick review! I'll add some background explanation before landing.
Comment 5 Hajime Morrita 2011-12-05 03:15:59 PST
Committed r101983: <http://trac.webkit.org/changeset/101983>
Comment 6 Iain Merrick 2011-12-05 07:02:18 PST
I think this is causing a crash in debug builds. ~Node now calls Node::document(), which includes the following: ASSERT(m_document || (nodeType() == DOCUMENT_TYPE_NODE && !inDocument())); Node::nodeType() is pure virtual and there cannot be called in a destructor (the subclass has already been destroyed). Chromium crashes on startup with a "pure virtual method called" error. I assume this affects all WebKit ports. I'll verify with a debug build of DumpRenderTree.
Comment 7 Iain Merrick 2011-12-05 07:09:55 PST
Aha, already rolled out in http://trac.webkit.org/changeset/101995
Comment 8 Csaba Osztrogonác 2011-12-05 07:28:11 PST
(In reply to comment #7) > Aha, already rolled out in http://trac.webkit.org/changeset/101995 It was rolled out because it broke all tests on GTK and on Qt in debug mode. The exact reason can be found in comment #6.
Comment 9 Csaba Osztrogonác 2011-12-05 07:28:29 PST
Comment on attachment 117851 [details] Patch removing flags from rolled out patch
Comment 10 Alexey Proskuryakov 2011-12-05 10:26:20 PST
> Node::document() is going to be a bit nontrivial What is the plan for Node::document()? It directly maps to DOM concept, so changing it could introduce a lot of complexity.
Comment 11 Hajime Morrita 2011-12-05 22:10:41 PST
I'm totally sorry about the last change. It was a disaster... By the way, > What is the plan for Node::document()? It directly maps to DOM concept, so changing it could introduce a lot of complexity. Basically, this series of change is adding an indirection for Node::document() call. This is preparation for Bug 59816, where I'm going to turn m_document into m_treeScope pointing TreeScope* instance, which is a superclass of Document and ShadowRoot. After the change, Node::document() will be something like: return m_treeScope->enclosingDocument(); TreeScope::m_enclosingDocument will point itself when the real class of the instance is Document. return its containing Document. Currently all elements in shadow have ElementRareData because they need ElementRareData::m_treeScope. This change eliminates ElementRareData::m_treeScope, so these nodes no longer need to have its rare data and can save memory allocation for that.
Comment 12 Hajime Morrita 2011-12-05 23:02:29 PST
Created attachment 117993 [details] A fixed version
Comment 13 Kent Tamura 2011-12-05 23:25:33 PST
Comment on attachment 117993 [details] A fixed version ok. Please watch the buildbots after landing the patch.
Comment 14 WebKit Review Bot 2011-12-06 03:31:01 PST
Comment on attachment 117993 [details] A fixed version Clearing flags on attachment: 117993 Committed r102119: <http://trac.webkit.org/changeset/102119>
Comment 15 WebKit Review Bot 2011-12-06 03:31:06 PST
All reviewed patches have been landed. Closing bug.
Comment 16 Lucas Forschler 2019-02-06 09:02:59 PST
Mass moving XML DOM bugs to the "DOM" Component.