RenderQuote::rendererSubtreeAttached is called on every invocation of RenderObjectChildList::appendChildNode and RenderObjectChildList::insertChildNode. This function walks through every descendant and calls a virtual function on it. This can be written way more smartly since 99.999% of the time, we don't have any renderquote in our tree. Same case for RenderCounters. void RenderQuote::rendererSubtreeAttached(RenderObject* renderer) { if (renderer->documentBeingDestroyed()) return; for (RenderObject* descendant = renderer; descendant; descendant = descendant->nextInPreOrder(renderer)) if (descendant->isQuote()) { toRenderQuote(descendant)->placeQuote(); break; } }
As mentioned in Abhishek's comment (nearly missed it), RenderCounter has the same pattern and thus the same penalty. Also the same applies to RenderObjectChildList::removeChildNode + rendererRemovedFromTree. Renaming to better broaden the issue.
Maybe we can put a flag or counter in the RenderView to turn this on and off?
(In reply to comment #2) > Maybe we can put a flag or counter in the RenderView to turn this on and off? That would have been my first idea as counters / quotes don't cross frames' boundaries. The other option is to add some static count in RenderCounter / RenderQuote so that we keep track when we have any of those. The second option seems easier to implement efficiently IMHO but it has the downside of being less efficient as it will cross frames' boundaries and be confused by history items (as we hold onto our render tree).
Created attachment 150452 [details] Patch Track the number of RenderQuote and RenderCounter in the RenderView
Attachment 150452 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:14: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 150452 [details] Patch Attachment 150452 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13127254
Comment on attachment 150452 [details] Patch Attachment 150452 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13135121
Comment on attachment 150452 [details] Patch Attachment 150452 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13124232
Comment on attachment 150452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150452&action=review If we move a node from one RenderView to another (maybe with adoptNode?), do we create new RenderObjects on the second RenderView? > Source/WebCore/rendering/RenderView.h:247 > + void addRenderCounter() { m_renderCounterCount++; } > + void removeRenderCounter() { m_renderCounterCount++; } Where is add/remove for RenderQuotes? removeRenderCounter looks like it should decrement.
Comment on attachment 150452 [details] Patch Attachment 150452 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13126243
Created attachment 150454 [details] Patch Fix busted compile...
(In reply to comment #9) > (From update of attachment 150452 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150452&action=review > > If we move a node from one RenderView to another (maybe with adoptNode?), do we create new RenderObjects on the second RenderView? I don't think we ever reuse renderers in different views so this shouldn't be an issue. > > > Source/WebCore/rendering/RenderView.h:247 > > + void addRenderCounter() { m_renderCounterCount++; } > > + void removeRenderCounter() { m_renderCounterCount++; } > > Where is add/remove for RenderQuotes? removeRenderCounter looks like it should decrement. Sorry, I busted the patch moving code around before I uploaded. Fixed.
Comment on attachment 150454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150454&action=review This seems totally reasonable to me. I wonder if this is the right place to store these values however. documentRareData is what first came to mind: http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.h#L436 but it seems this is rendering specific, so maybe RenderView isn't a bad place to hold this. > Source/WebCore/rendering/RenderQuote.cpp:60 > + document()->renderView()->addRenderQuote(); Doesn't view() get you this directly?
Comment on attachment 150454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150454&action=review > Source/WebCore/rendering/RenderCounter.cpp:661 > + if (!renderer->view()->hasRenderCounters()) Why not call this at start of function. > Source/WebCore/rendering/RenderQuote.cpp:296 > if (subtreeRoot->documentBeingDestroyed()) I don't like the name 'subtreeRoot' in RenderQuote::rendererRemovedFromTree and 'removedRenderer' in RenderCounter::rendererRemovedFromTree. Just keep it simple and name it 'renderer' as in rendererSubtreeAttached. What do you think ? > Source/WebCore/rendering/RenderView.h:200 > + void removeRenderQuote() { m_renderQuoteCount++; } Typo s/++/-- > Source/WebCore/rendering/RenderView.h:201 > + bool hasRenderQuotes() { return m_renderQuoteCount > 0; } Why not just "return m_renderQuoteCount" > Source/WebCore/rendering/RenderView.h:203 > + void removeRenderCounter() { m_renderCounterCount++; } Typo s/++/-- > Source/WebCore/rendering/RenderView.h:204 > + bool hasRenderCounters() { return m_renderCounterCount > 0; } Why not just "return m_renderCounterCount"
Is it really a good idea to add this hack? render quote and counter are already piles of hacks. I'm not certain that adding more hacks to gain performance is going to help us. It may make the future refactoring harder.
Comment on attachment 150454 [details] Patch Attachment 150454 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13127279 New failing tests: accessibility/aria-checkbox-checked.html animations/3d/matrix-transform-type-animation.html http/tests/appcache/cyrillic-uri.html accessibility/anchor-linked-anonymous-block-crash.html animations/animation-add-events-in-handler.html http/tests/appcache/deferred-events-delete-while-raising.html animations/additive-transform-animations.html animations/3d/replace-filling-transform.html http/tests/appcache/deferred-events-delete-while-raising-timer.html accessibility/aria-disabled.html http/tests/appcache/crash-when-navigating-away-then-back.html animations/animation-border-overflow.html animations/animation-direction-alternate-reverse.html accessibility/accessibility-object-detached.html http/tests/appcache/credential-url.html animations/animation-controller-drt-api.html animations/3d/change-transform-in-end-event.html accessibility/aria-hidden.html http/tests/appcache/destroyed-frame.html canvas/philip/tests/2d.canvas.readonly.html animations/3d/transform-perspective.html accessibility/aria-hidden-updates-alldescendants.html http/tests/appcache/access-via-redirect.php http/tests/appcache/deferred-events.html animations/3d/state-at-end-event-transform.html animations/3d/transform-origin-vs-functions.html animations/animation-css-rule-types.html animations/animation-direction-reverse-fill-mode-hardware.html accessibility/adjacent-continuations-cause-assertion-failure.html accessibility/aria-help.html
Created attachment 150463 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 150476 [details] Patch Different approach that uses willBeDestroyed and adds document destruction checks for cases where the RenderView may not exist.
(In reply to comment #14) > (From update of attachment 150454 [details]) > ... > > Source/WebCore/rendering/RenderQuote.cpp:296 > > if (subtreeRoot->documentBeingDestroyed()) > > I don't like the name 'subtreeRoot' in RenderQuote::rendererRemovedFromTree and 'removedRenderer' in RenderCounter::rendererRemovedFromTree. Just keep it simple and name it 'renderer' as in rendererSubtreeAttached. What do you think ? That involves changing a bunch of lines unrelated to my change? Should I just do it anyway? I agree it's weird with the different names.
Comment on attachment 150476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150476&action=review It is ok to rename vars in this patch. > Source/WebCore/rendering/RenderCounter.cpp:671 > + if (!renderer->view()->hasRenderCounters()) Looks like you forgot to move this to start of function. > Source/WebCore/rendering/RenderQuote.cpp:289 > if (renderer->documentBeingDestroyed()) I think this documentBeingDestroyed in rendererSubtreeAttached is wrong. How can document be in destruction mode when doing appendChildNode. Same for RenderCounter. > Source/WebCore/rendering/RenderQuote.cpp:302 > if (subtreeRoot->documentBeingDestroyed()) Instead of doing documentBeingDestroyed for both RenderCounter and RenderQuote every time, why don't we put this back into the caller removeChildNode.
Created attachment 150490 [details] Patch Fix review comments
(In reply to comment #15) > Is it really a good idea to add this hack? render quote and counter are already piles of hacks. I'm not certain that adding more hacks to gain performance is going to help us. It may make the future refactoring harder. I agree the code is hacky, but no one seems to have taken up the task to rewrite it. Instead of waiting longer for that to happen it seems reasonable to "make the busted code run less often" which is what this patch does.
Comment on attachment 150490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150490&action=review > Source/WebCore/rendering/RenderQuote.cpp:289 > + if (!renderer->view()->hasRenderQuotes()) Don't we still want the renderer->documentBeingDestroyed() check here?
(In reply to comment #23) > (From update of attachment 150490 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150490&action=review > > > Source/WebCore/rendering/RenderQuote.cpp:289 > > + if (!renderer->view()->hasRenderQuotes()) > > Don't we still want the renderer->documentBeingDestroyed() check here? It was moved to the caller in removeChildNode to avoid repeating with both renderquote and rendercounter.
Comment on attachment 150490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150490&action=review Lets see how the EWS bots turn up. Can you run some any perf tests to see what gains we got ? > Source/WebCore/ChangeLog:8 > + Previous we'd walk the all children of the Render Tree whenever adding No need to abbreviate. s/we'd/we would s/node/renderer s/Tree/subtree. (since it is part of the tree under renderer.) Think about rephrasing this sentence. > Source/WebCore/ChangeLog:15 > + tests should cover it. There's no change in the Render tree or page layout so I'm No need of the 'There's no change... line' > Source/WebCore/ChangeLog:18 > + * rendering/RenderCounter.cpp: Please fill in the below sections.
Comment on attachment 150490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150490&action=review >>> Source/WebCore/rendering/RenderQuote.cpp:289 >>> + if (!renderer->view()->hasRenderQuotes()) >> >> Don't we still want the renderer->documentBeingDestroyed() check here? > > It was moved to the caller in removeChildNode to avoid repeating with both renderquote and rendercounter. This is during adding a child node though.
Comment on attachment 150490 [details] Patch Attachment 150490 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13133237 New failing tests: css2.1/t1202-counter-01-b.html css2.1/t1202-counters-14-b.html css2.1/t120403-visibility-00-c.html css2.1/t120403-content-none-00-c.html css2.1/t1202-counters-16-c.html css2.1/t1202-counter-07-b.html fast/block/line-layout/line-break-removal-near-textarea-crash.html css2.1/t1202-counters-12-b.html css2.1/t120401-scope-01-c.html css2.1/t120401-scope-03-c.html css2.1/t1204-order-00-c.html css2.1/t1204-implied-01-c.html accessibility/crash-with-noelement-selectbox.html css2.1/t1202-counters-09-b.html css2.1/t1202-counter-16-f.html css2.1/t1202-counters-03-b.html css2.1/t1202-counter-12-b.html fast/css/begin-end-contain-selector-empty-value.html css2.1/t1202-counters-07-b.html css2.1/t1202-counter-09-b.html css2.1/t1202-counter-14-b.html css2.1/t1202-counters-01-b.html css2.1/t1204-multiple-00-c.html css2.1/t1202-counter-05-b.html css2.1/t1202-counter-03-b.html css2.1/t1202-counters-18-f.html css2.1/t1204-root-e.html css2.1/t1202-counters-05-b.html
Created attachment 150501 [details] Archive of layout-test-results from gce-cr-linux-08 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 150502 [details] Patch Clean up ChangeLog.
Comment on attachment 150502 [details] Patch Attachment 150502 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13130286 New failing tests: css2.1/t1202-counter-01-b.html css2.1/t1202-counters-14-b.html css2.1/t120403-visibility-00-c.html css2.1/t120403-content-none-00-c.html css2.1/t1202-counters-16-c.html css2.1/t1202-counter-07-b.html fast/block/line-layout/line-break-removal-near-textarea-crash.html css2.1/t1202-counters-12-b.html css2.1/t120401-scope-01-c.html css2.1/t120401-scope-03-c.html css2.1/t1204-order-00-c.html css2.1/t1204-implied-01-c.html accessibility/crash-with-noelement-selectbox.html css2.1/t1202-counters-09-b.html css2.1/t1202-counter-16-f.html css2.1/t1202-counters-03-b.html css2.1/t1202-counter-12-b.html fast/css/begin-end-contain-selector-empty-value.html css2.1/t1202-counters-07-b.html css2.1/t1202-counter-09-b.html css2.1/t1202-counter-14-b.html css2.1/t1202-counters-01-b.html css2.1/t1204-multiple-00-c.html css2.1/t1202-counter-05-b.html css2.1/t1202-counter-03-b.html css2.1/t1202-counters-18-f.html css2.1/t1204-root-e.html css2.1/t1202-counters-05-b.html
Created attachment 150504 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 150511 [details] Patch Avoid accessing view() in willBeDestroyed() when the document itself is being destroyed
(In reply to comment #26) > (From update of attachment 150490 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=150490&action=review > > >>> Source/WebCore/rendering/RenderQuote.cpp:289 > >>> + if (!renderer->view()->hasRenderQuotes()) > >> > >> Don't we still want the renderer->documentBeingDestroyed() check here? > > > > It was moved to the caller in removeChildNode to avoid repeating with both renderquote and rendercounter. > > This is during adding a child node though. Why would we be attaching a renderer while the whole document is being destroyed? I don't understand what you mean.
Comment on attachment 150511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150511&action=review Looks good. Lets leave the patch for today so that the other cced reviewers can comment if needed. > Source/WebCore/rendering/RenderQuote.cpp:69 > + if (!documentBeingDestroyed()) { The comment is not required and this could be better read as if (RenderView* view = this->view()) view->removeRenderQuote(); since ALWAYS_INLINE RenderView* RenderObject::view() const { return toRenderView(document()->renderer()); } and inline bool RenderObject::documentBeingDestroyed() const { return !document()->renderer(); } Same change for RenderCounter.
If anyone was waiting for my comment on this, here it is: I have looked through the patch and in general looks good to me. I do not particularly like the // FIXME comment. There isn't much to fix. The code is good as is, there may be ways to improve it, but last time I checked this was a decent implementation of the standard. To me this patch is a performance improvement not a workaround. My only real concern is that there are no checks against the view() being always available and correct when needed. Sometimes entire renderer subtrees are being detached and reparented. I am not sure what happens with the view() in this case. If it is a truism that the view is right and not NULL in all cases when accessed, then, great, if not, maybe an ASSERT would be warranted, if we are even less certain, maybe a handling of that case in all builds would be needed.
(In reply to comment #35) > If anyone was waiting for my comment on this, here it is: > I have looked through the patch and in general looks good to me. > I do not particularly like the // FIXME comment. There isn't much to fix. The code is good as is, there may be ways to improve it, but last time I checked this was a decent implementation of the standard. To me this patch is a performance improvement not a workaround. > My only real concern is that there are no checks against the view() being always available and correct when needed. Sometimes entire renderer subtrees are being detached and reparented. I am not sure what happens with the view() in this case. > If it is a truism that the view is right and not NULL in all cases when accessed, then, great, if not, maybe an ASSERT would be warranted, if we are even less certain, maybe a handling of that case in all builds would be needed. The FIXME is important as currently we don't have a good workaround for this in cases where we have atleast one counter or quote in the rendertree. In that case, we will still walk every descendant in the tree to look for that one sneaky counter / quote on every child addition/deletion. Ouch :( We still need to think about ways to prevent that. This patch is a immediate first step. And the FIXME is a reminder to fix the other case and possibly get rid of the counting code in RenderView
(In reply to comment #36) > (In reply to comment #35) > The FIXME is important as currently we don't have a good workaround for this in cases where we have atleast one counter or quote in the rendertree. In that case, we will still walk every descendant in the tree to look for that one sneaky counter / quote on every child addition/deletion. Ouch :( We still need to think I did not realize that. When I originally wrote this code, we would only walk the render tree (not all of it, but I admit a decent chunk of it) only when a renderer subtree that contains a counter or a quote is added or removed. Yes, for every renderer subtree that is being attached or detached, we check that subtree (which in all practical cases that I saw contained less than 10 objects, usually 1) to check if there is a counter or a quote in it. If there is no counter or quote we do nothing more. If there is a counter or quote in the subtree that is being removed or added, then we walk the tree till we find its ancestor or till we hit the root. Even in that case, we usually do not walk the whole tree, there are optimizations in the code (or at least there were when I last looked at it) that cause only the necessary nodes to be walked, skipping entire subtrees when that is possible. Really, if you and others believe that the counters code is causing WebKit to be slower than it should, I have no objection to the FIXME. I know it used to cause big trouble with laying out, because the rules for counters and quotes can cause changes in unexpected parts of the tree, there were a few proposals for fixing that. I am not sure where they ended up. This code is not my baby, someone smarter than me designed it before I got involved, I only brought some finishing touches to it. I am just trying to state my understanding and hopefully useful honest opinion about the patch. By the way, if you guys really want to be thorough, it would probably help to do a performance check. Get a document without counters, or quotes. Add and drop nodes with no counters or quotes in them and see how much this patch speeds up things. Unless things changed dramatically since I last looked at this code, you should not see much of a difference. The time needed to layout the tree, would make the time needed to walk those tiny subtrees that are being attached and detached negligible. Even if there are many nodes added and removed without an intervenning layout, the time to check the tiny trees that are normally added and removed, should be negligible compared to the time to run the JS to construct the nodes. This patch really helps in the case when large subtrees of renderers are moved around in the document. What I saw was that usually when DOM Nodes are moved, the renderers are destroyed and recreated, then attached in small groups, but my knowledge can be obsolete now.
Created attachment 152047 [details] Patch Updated with comments from inferno
Comment on attachment 152047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152047&action=review > Source/WebCore/rendering/RenderView.h:201 > + bool hasRenderQuotes() { return m_renderQuoteCount; } Some compilers might prefer you to write "return !!m_renderQuoteCount;" here, but I doubt it actually matters.
(In reply to comment #39) > (From update of attachment 152047 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152047&action=review > > > Source/WebCore/rendering/RenderView.h:201 > > + bool hasRenderQuotes() { return m_renderQuoteCount; } > > Some compilers might prefer you to write "return !!m_renderQuoteCount;" here, but I doubt it actually matters. Yeah, I was aware of that and originally had m_renderQuoteCount > 0, but Abhishek said to just return the value directly. Should I change this back?
Comment on attachment 152047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152047&action=review >>> Source/WebCore/rendering/RenderView.h:201 >>> + bool hasRenderQuotes() { return m_renderQuoteCount; } >> >> Some compilers might prefer you to write "return !!m_renderQuoteCount;" here, but I doubt it actually matters. > > Yeah, I was aware of that and originally had m_renderQuoteCount > 0, but Abhishek said to just return the value directly. > > Should I change this back? I'm pretty sure we do this all over the place. Arguably the style guide even requires not comparing to 0 or !!'ing. I can't imagine any compiler we cares about requires this.
Comment on attachment 152047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152047&action=review I'm ready to r+ this, but it would be nice to see your answers to my below questions. > Source/WebCore/ChangeLog:26 > + (WebCore::RenderObjectChildList::removeChildNode): Short circuit calling into Counter and Quote code when the document is being destroyed. This is a perf win? > Source/WebCore/rendering/RenderCounter.cpp:610 > + if (!renderer->view()->hasRenderCounters()) > + return; rendererRemovedFromTree must be a static method, otherwise this check doesn't make any sense. :) (mostly thinking out loud here.) > Source/WebCore/rendering/RenderObjectChildList.cpp:160 > + if (!owner->documentBeingDestroyed()) { > + RenderCounter::rendererRemovedFromTree(oldChild); > + RenderQuote::rendererRemovedFromTree(oldChild); > + } You might add a comment right before this which says: // We can skip tracking counter/quote removals when the whole document is being destroyed for a small speedup. or similar. It's nice to know the "why" when reading the code. > Source/WebCore/rendering/RenderQuote.cpp:70 > + if (view()) > + view()->removeRenderQuote(); Do you not have a view when you're being destroyed? Or maybe when you were created but never inserted? Just curious why the null-check. > Source/WebCore/rendering/RenderView.cpp:66 > + , m_renderQuoteCount(0) > + , m_renderCounterCount(0) Just checking that this is the only Constructor we need to worry about for this class... I expect it is.
Created attachment 152053 [details] Patch Updated with comments from Eric Sidel
(In reply to comment #42) > (From update of attachment 152047 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152047&action=review > > I'm ready to r+ this, but it would be nice to see your answers to my below questions. > > > Source/WebCore/ChangeLog:26 > > + (WebCore::RenderObjectChildList::removeChildNode): Short circuit calling into Counter and Quote code when the document is being destroyed. > > This is a perf win? It should be, otherwise we walk all over the tree when destroying the document. These checks already existed, but they were in each class instead of in that single location. I don't know the real impact this has on perf. > > > Source/WebCore/rendering/RenderCounter.cpp:610 > > + if (!renderer->view()->hasRenderCounters()) > > + return; > > rendererRemovedFromTree must be a static method, otherwise this check doesn't make any sense. :) (mostly thinking out loud here.) Yeah it is. > > > Source/WebCore/rendering/RenderObjectChildList.cpp:160 > > + if (!owner->documentBeingDestroyed()) { > > + RenderCounter::rendererRemovedFromTree(oldChild); > > + RenderQuote::rendererRemovedFromTree(oldChild); > > + } > > You might add a comment right before this which says: // We can skip tracking counter/quote removals when the whole document is being destroyed for a small speedup. or similar. It's nice to know the "why" when reading the code. Added. > > > Source/WebCore/rendering/RenderQuote.cpp:70 > > + if (view()) > > + view()->removeRenderQuote(); > > Do you not have a view when you're being destroyed? Or maybe when you were created but never inserted? Just curious why the null-check. If the document is being destroyed your view is destroyed before you are and this would cause a crash. Originally this code read as if (!documentBeingDestroyed()) view()->... which I think was a little more clear, but inferno@ thought it was better written like this. I don't think this can happen if you're created and not inserted since willBeDestroyed() is part of the renderer lifecycle inside the tree. > > > Source/WebCore/rendering/RenderView.cpp:66 > > + , m_renderQuoteCount(0) > > + , m_renderCounterCount(0) > > Just checking that this is the only Constructor we need to worry about for this class... I expect it is. RenderView only has one constructor declared in RenderView.h
(In reply to comment #44) > (In reply to comment #42) > > (From update of attachment 152047 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=152047&action=review > > > > I'm ready to r+ this, but it would be nice to see your answers to my below questions. > > > > > Source/WebCore/ChangeLog:26 > > > + (WebCore::RenderObjectChildList::removeChildNode): Short circuit calling into Counter and Quote code when the document is being destroyed. > > > > This is a perf win? > > It should be, otherwise we walk all over the tree when destroying the document. These checks already existed, but they were in each class instead of in that single location. I don't know the real impact this has on perf. Also this prevents crashes when doing the hasRenderQuotes|Counters() check, since you couldn't access view() when the document is being destroyed. I suppose there should be an ASSERT(view()) at the top of rendererRemovedFromTree.
Comment on attachment 152047 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152047&action=review >>>> Source/WebCore/ChangeLog:26 >>>> + (WebCore::RenderObjectChildList::removeChildNode): Short circuit calling into Counter and Quote code when the document is being destroyed. >>> >>> This is a perf win? >> >> It should be, otherwise we walk all over the tree when destroying the document. These checks already existed, but they were in each class instead of in that single location. I don't know the real impact this has on perf. > > Also this prevents crashes when doing the hasRenderQuotes|Counters() check, since you couldn't access view() when the document is being destroyed. > > I suppose there should be an ASSERT(view()) at the top of rendererRemovedFromTree. I like ASSERTS. :) (They're even better than comments!)
Comment on attachment 152053 [details] Patch I like this patch. Thanks.
Created attachment 152061 [details] Patch for landing
Created attachment 152077 [details] Patch for landing
Comment on attachment 152077 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=152077&action=review > Source/WebCore/rendering/RenderCounter.h:44 > + virtual void willBeDestroyed(); Nit: New virtual functions should be annotated as OVERRIDE when it makes sense (like here and in RenderQuote::willBeDestroyed).
Comment on attachment 152077 [details] Patch for landing Clearing flags on attachment: 152077 Committed r122524: <http://trac.webkit.org/changeset/122524>
All reviewed patches have been landed. Closing bug.
ClusterFuzz has come to our rescue. This patch is causing lot of null ptr crashes during insertChildNode. You remember we added the documentBeingDestroyed check during removeChildNode, but not during insertChildNode. The problem is when document is getting destructed, we can run into cases where insertChildNode is called. A classic example is RenderBlock::removeChild which merges empty anonymous block into its neighbouring block(causing children to be moved) (IDEALLY we shouldn't be doing this (slow operation)), but there might be similar cases. As i am thinking, we don't really need to track counters/quotes in that situation, since those ones will get destroyed later (as part of same document destruction). Testcase:: <script>if (window.layoutTestController) layoutTestController.waitUntilDone(); </script> <style> .c6::after { float: left; content: open-quote;</style> <script> var nodes = Array(); var text = Array(); function boom() { try { nodes[22] = document.createElement('figure'); } catch(e) {} try { nodes[56] = document.createElement('header'); } catch(e) {} try { nodes[56].setAttribute('class', 'c6'); } catch(e) {} try { document.documentElement.appendChild(nodes[56]); } catch(e) {} try { text[32] = document.createTextNode('hssh'); } catch(e) {} try { nodes[56].appendChild(nodes[22]); } catch(e) {} try { nodes[56].appendChild(text[32]); } catch(e) {} } window.onload = boom; </script> <meta http-equiv="refresh" content="0"</head> +----------------------------------------Debug Build Stacktrace----------------------------------------+ /mnt/scratch0/clusterfuzz/slave-bot/builds/symbolized/debug/asan-linux-debug-146683/DumpRenderTree ASSERTION FAILED: renderer->view() third_party/WebKit/Source/WebCore/rendering/RenderCounter.cpp(661) : static void WebCore::RenderCounter::rendererSubtreeAttached(WebCore::RenderObject *) 1 0x7f53547c339c 2 0x7f5354c18ff3 3 0x7f53547555dd 4 0x7f53547561b2 5 0x7f535447b0ce 6 0x7f53543b7444 7 0x7f53543b90d2 8 0x7f53549035dd 9 0x7f5354bfea1d 10 0x7f53546fd1e3 11 0x7f535465d489 12 0x7f53543a1447 13 0x7f5354bff885 14 0x7f5354bff295 15 0x7f534deee149 16 0x7f534d999b45 17 0x7f534dcf9145 18 0x7f534d9a7771 19 0x7f534d999b2b 20 0x7f534dcf9145 21 0x7f534d9a7771 22 0x7f534d999b2b 23 0x7f534dcf9145 24 0x7f534d9a7771 25 0x7f534d999b2b 26 0x7f534da71f58 27 0x7f534da72ef4 28 0x7f5353af6fce 29 0x7f5353b04c3f 30 0x7f5348546387 31 0x7f534822ccf4 ASAN:SIGSEGV ==31854== ERROR: AddressSanitizer crashed on unknown address 0x0000bbadbeef (pc 0x7f53547c33c4 sp 0x7fff77227000 bp 0x7fff77227190 T0) AddressSanitizer can not provide additional info. ABORTING #0 0x7f53547c33c4 in WebCore::RenderCounter::rendererSubtreeAttached(WebCore::RenderObject*) third_party/WebKit/Source/WebCore/rendering/RenderCounter.cpp:661 #1 0x7f5354c18ff3 in WebCore::RenderObjectChildList::insertChildNode(WebCore::RenderObject*, WebCore::RenderObject*, WebCore::RenderObject*, bool) third_party/WebKit/Source/WebCore/rendering/RenderObjectChildList.cpp:285 #2 0x7f53547555dd in WebCore::RenderBoxModelObject::moveChildTo(WebCore::RenderBoxModelObject*, WebCore::RenderObject*, WebCore::RenderObject*, bool) third_party/WebKit/Source/WebCore/rendering/RenderBoxModelObject.cpp:2774 #3 0x7f53547561b2 in WebCore::RenderBoxModelObject::moveChildrenTo(WebCore::RenderBoxModelObject*, WebCore::RenderObject*, WebCore::RenderObject*, WebCore::RenderObject*, bool) third_party/WebKit/Source/WebCore/rendering/RenderBoxModelObject.cpp:2792 #4 0x7f535447b0ce in WebCore::RenderBoxModelObject::moveAllChildrenTo(WebCore::RenderBoxModelObject*, WebCore::RenderObject*, bool) third_party/WebKit/Source/WebCore/rendering/RenderBoxModelObject.h:265 #5 0x7f53543b7444 in WebCore::RenderBlock::collapseAnonymousBoxChild(WebCore::RenderBlock*, WebCore::RenderObject*) third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1142 #6 0x7f53543b90d2 in WebCore::RenderBlock::removeChild(WebCore::RenderObject*) third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1216 #7 0x7f53549035dd in WebCore::RenderObject::remove() third_party/WebKit/Source/WebCore/rendering/RenderObject.h:874 #8 0x7f5354bfea1d in WebCore::RenderObject::willBeDestroyed() third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:2310 #9 0x7f53546fd1e3 in WebCore::RenderBoxModelObject::willBeDestroyed() third_party/WebKit/Source/WebCore/rendering/RenderBoxModelObject.cpp:358 #10 0x7f535465d489 in WebCore::RenderBox::willBeDestroyed() third_party/WebKit/Source/WebCore/rendering/RenderBox.cpp:149 #11 0x7f53543a1447 in WebCore::RenderBlock::willBeDestroyed() third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:276 #12 0x7f5354bff885 in WebCore::RenderObject::destroy() third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:2371 #13 0x7f5354bff295 in WebCore::RenderObject::destroyAndCleanupAnonymousWrappers() third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:2348 #14 0x7f534deee149 in WebCore::Node::detach() third_party/WebKit/Source/WebCore/dom/Node.cpp:1289 #15 0x7f534d999b45 in WebCore::ContainerNode::detach() third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:676 #16 0x7f534dcf9145 in WebCore::Element::detach() third_party/WebKit/Source/WebCore/dom/Element.cpp:993 #17 0x7f534d9a7771 in WebCore::ContainerNode::detachChildren() third_party/WebKit/Source/WebCore/dom/ContainerNode.h:203 #18 0x7f534d999b2b in WebCore::ContainerNode::detach() third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:674 #19 0x7f534dcf9145 in WebCore::Element::detach() third_party/WebKit/Source/WebCore/dom/Element.cpp:993 #20 0x7f534d9a7771 in WebCore::ContainerNode::detachChildren() third_party/WebKit/Source/WebCore/dom/ContainerNode.h:203 #21 0x7f534d999b2b in WebCore::ContainerNode::detach() third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:674 #22 0x7f534dcf9145 in WebCore::Element::detach() third_party/WebKit/Source/WebCore/dom/Element.cpp:993 #23 0x7f534d9a7771 in WebCore::ContainerNode::detachChildren() third_party/WebKit/Source/WebCore/dom/ContainerNode.h:203 #24 0x7f534d999b2b in WebCore::ContainerNode::detach() third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:674 #25 0x7f534da71f58 in WebCore::Document::detach() third_party/WebKit/Source/WebCore/dom/Document.cpp:2129 #26 0x7f534da72ef4 in WebCore::Document::prepareForDestruction() third_party/WebKit/Source/WebCore/dom/Document.cpp:2149 #27 0x7f5353af6fce in WebCore::Frame::setView(WTF::PassRefPtr<WebCore::FrameView>) third_party/WebKit/Source/WebCore/page/Frame.cpp:271 #28 0x7f5353b04c3f in WebCore::Frame::createView(WebCore::IntSize const&, WebCore::Color const&, bool, WebCore::IntSize const&, bool, WebCore::ScrollbarMode, bool, WebCore::ScrollbarMode, bool) third_party/WebKit/Source/WebCore/page/Frame.cpp:805 #29 0x7f5348546387 in WebKit::WebFrameImpl::createFrameView() third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:2151 #30 0x7f534822ccf4 in WebKit::FrameLoaderClientImpl::makeDocumentView() third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:273 #31 0x7f5348243ba3 in WebKit::FrameLoaderClientImpl::transitionToCommittedForNewPage() third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1409 #32 0x7f5353650460 in WebCore::FrameLoader::transitionToCommitted(WTF::PassRefPtr<WebCore::CachedPage>) third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:1805 #33 0x7f535364e05d in WebCore::FrameLoader::commitProvisionalLoad() third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:1663 #34 0x7f5353551b76 in WebCore::DocumentLoader::commitIfReady() third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:283 #35 0x7f53535530b5 in WebCore::DocumentLoader::commitLoad(char const*, int) third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:314 #36 0x7f5353553cab in WebCore::DocumentLoader::receivedData(char const*, int) third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:366 #37 0x7f53536c926a in WebCore::MainResourceLoader::addData(char const*, int, bool) third_party/WebKit/Source/WebCore/loader/MainResourceLoader.cpp:193 #38 0x7f53537597db in WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:276 #39 0x7f53536ce2d9 in WebCore::MainResourceLoader::didReceiveData(char const*, int, long long, bool) third_party/WebKit/Source/WebCore/loader/MainResourceLoader.cpp:484 #40 0x7f535375cd5f in WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:431 #41 0x7f53506fac19 in WebCore::ResourceHandleInternal::didReceiveData(WebKit::WebURLLoader*, char const*, int, int) third_party/WebKit/Source/WebCore/platform/network/chromium/ResourceHandle.cpp:139 #42 0x7f5338bf0411 in webkit_glue::WebURLLoaderImpl::Context::OnReceivedData(char const*, int, int) webkit/glue/weburlloader_impl.cc:615 #43 0xe914a4 in (anonymous namespace)::RequestProxy::NotifyReceivedData(int) webkit/tools/test_shell/simple_resource_loader_bridge.cc:380 #44 0xe93622 in base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int)>::Run((anonymous namespace)::RequestProxy*, int const&) ./base/bind_internal.h:190 #45 0xe93282 in base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int)>, void ()((anonymous namespace)::RequestProxy* const&, int const&)>::MakeItSo(base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int)>, (anonymous namespace)::RequestProxy* const&, int const&) ./base/bind_internal.h:899 #46 0xe92ead in base::internal::Invoker<2, base::internal::BindState<base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int)>, void ()((anonymous namespace)::RequestProxy*, int), void ()((anonymous namespace)::RequestProxy*, int)>, void ()((anonymous namespace)::RequestProxy*, int)>::Run(base::internal::BindStateBase*) ./base/bind_internal.h:1256 #47 0x7f5340b7c935 in base::Callback<void ()()>::Run() const ./base/callback.h:388 #48 0x7f5340d97f5f in MessageLoop::RunTask(base::PendingTask const&) base/message_loop.cc:457 #49 0x7f5340d998c3 in MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) base/message_loop.cc:471 #50 0x7f5340d9a0e8 in MessageLoop::DoWork() base/message_loop.cc:644 #51 0x7f5340a5482c in base::MessagePumpGlib::RunWithDispatcher(base::MessagePump::Delegate*, base::MessagePumpDispatcher*) base/message_pump_glib.cc:203 #52 0x7f5340a56f45 in base::MessagePumpGlib::Run(base::MessagePump::Delegate*) base/message_pump_glib.cc:292 #53 0x7f5340d967ad in MessageLoop::RunInternal() base/message_loop.cc:416 #54 0x7f5340d962c3 in MessageLoop::RunHandler() base/message_loop.cc:389 #55 0x7f5340f53a54 in base::RunLoop::Run() base/run_loop.cc:46 #56 0x7f5340d94117 in MessageLoop::Run() base/message_loop.cc:300 #57 0x927b81 in webkit_support::RunMessageLoop() webkit/support/webkit_support.cc:518 #58 0x69cd76 in TestShell::waitTestFinished() third_party/WebKit/Tools/DumpRenderTree/chromium/TestShellPosix.cpp:66 #59 0x657426 in TestShell::runFileTest(TestParams const&) third_party/WebKit/Tools/DumpRenderTree/chromium/TestShell.cpp:274 #60 0x4d91ac in runTest(TestShell&, TestParams&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, bool) third_party/WebKit/Tools/DumpRenderTree/chromium/DumpRenderTree.cpp:128 #61 0x4d6e0d in main third_party/WebKit/Tools/DumpRenderTree/chromium/DumpRenderTree.cpp:274 #62 0x7f533282dc4d in __libc_start_main /build/buildd/eglibc-2.11.1/csu/libc-start.c:258 Stats: 13M malloced (24M for red zones) by 84577 calls Stats: 1M realloced by 1420 calls Stats: 11M freed by 66821 calls Stats: 0M really freed by 0 calls Stats: 68M (17417 full pages) mmaped in 17 calls mmaps by size class: 8:81915; 9:8191; 10:4095; 11:2047; 12:1024; 13:512; 14:256; 15:128; 16:64; 17:32; 18:32; 19:8; mallocs by size class: 8:78014; 9:3475; 10:1580; 11:739; 12:316; 13:145; 14:196; 15:72; 16:15; 17:5; 18:18; 19:2; frees by size class: 8:62122; 9:2096; 10:1306; 11:613; 12:282; 13:127; 14:179; 15:63; 16:9; 17:4; 18:18; 19:2; rfrees by size class: Stats: malloc large: 25 small slow: 281 +----------------------------------------Release Build Stacktrace----------------------------------------+ /mnt/scratch0/clusterfuzz/slave-bot/builds/symbolized/release/asan-symbolized-linux-release-146683/DumpRenderTree ASAN:SIGSEGV ==31771== ERROR: AddressSanitizer crashed on unknown address 0x00000000015c (pc 0x00000223bde1 sp 0x7fffa1232d20 bp 0x7fffa1232d20 T0) AddressSanitizer can not provide additional info. ABORTING #0 0x223bde1 in WebCore::RenderView::hasRenderCounters() third_party/WebKit/Source/WebCore/rendering/RenderView.h:204 #1 0x223be17 in WebCore::RenderCounter::rendererSubtreeAttached(WebCore::RenderObject*) third_party/WebKit/Source/WebCore/rendering/RenderCounter.cpp:662 #2 0x2163ccb in WebCore::RenderObjectChildList::insertChildNode(WebCore::RenderObject*, WebCore::RenderObject*, WebCore::RenderObject*, bool) third_party/WebKit/Source/WebCore/rendering/RenderObjectChildList.cpp:285 #3 0x2081e01 in WebCore::RenderBoxModelObject::moveChildrenTo(WebCore::RenderBoxModelObject*, WebCore::RenderObject*, WebCore::RenderObject*, WebCore::RenderObject*, bool) third_party/WebKit/Source/WebCore/rendering/RenderBoxModelObject.cpp:2790 #4 0x1faac4a in WebCore::RenderBlock::collapseAnonymousBoxChild(WebCore::RenderBlock*, WebCore::RenderObject*) third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1142 #5 0x1fab23e in WebCore::RenderBlock::removeChild(WebCore::RenderObject*) third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:1218 #6 0x21603c7 in WebCore::RenderObject::willBeDestroyed() third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:2326 #7 0x1fa6788 in WebCore::RenderBlock::willBeDestroyed() third_party/WebKit/Source/WebCore/rendering/RenderBlock.cpp:276 #8 0x21606de in WebCore::RenderObject::destroy() third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:2371 #9 0x97842c in WebCore::Node::detach() third_party/WebKit/Source/WebCore/dom/Node.cpp:1290 #10 0x942197 in WebCore::Element::detach() third_party/WebKit/Source/WebCore/dom/Element.cpp:993 #11 0x8cc539 in WebCore::ContainerNode::detachChildren() third_party/WebKit/Source/WebCore/dom/ContainerNode.h:203 #12 0x8cc4be in WebCore::ContainerNode::detach() third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:674 #13 0x942197 in WebCore::Element::detach() third_party/WebKit/Source/WebCore/dom/Element.cpp:993 #14 0x8cc539 in WebCore::ContainerNode::detachChildren() third_party/WebKit/Source/WebCore/dom/ContainerNode.h:203 #15 0x8cc4be in WebCore::ContainerNode::detach() third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:674 #16 0x942197 in WebCore::Element::detach() third_party/WebKit/Source/WebCore/dom/Element.cpp:993 #17 0x8cc539 in WebCore::ContainerNode::detachChildren() third_party/WebKit/Source/WebCore/dom/ContainerNode.h:203 #18 0x8cc4be in WebCore::ContainerNode::detach() third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:674 #19 0x8eeeed in WebCore::Document::detach() third_party/WebKit/Source/WebCore/dom/Document.cpp:2129 #20 0x1c266ed in WebCore::Frame::setView(WTF::PassRefPtr<WebCore::FrameView>) third_party/WebKit/Source/WebCore/page/Frame.cpp:271 #21 0x1c2964a in ~PassRefPtr third_party/WebKit/Source/WTF/wtf/PassRefPtr.h:67 #22 0x5ed56a in WebKit::WebFrameImpl::createFrameView() third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:2151 #23 0x1b5c056 in WebCore::FrameLoader::transitionToCommitted(WTF::PassRefPtr<WebCore::CachedPage>) third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:1816 #24 0x1b5b248 in ~PassRefPtr third_party/WebKit/Source/WTF/wtf/PassRefPtr.h:67 #25 0x1b2a87c in WebCore::DocumentLoader::commitLoad(char const*, int) third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:314 #26 0x1b8b0b2 in WebCore::ResourceLoader::didReceiveData(char const*, int, long long, bool) third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:276 #27 0x1b7526c in void WTF::derefIfNotNull<WebCore::MainResourceLoader>(WebCore::MainResourceLoader*) third_party/WebKit/Source/WTF/wtf/PassRefPtr.h:52 #28 0x1b8be44 in WebCore::ResourceLoader::didReceiveData(WebCore::ResourceHandle*, char const*, int, int) third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:431 #29 0x2b6774d in (anonymous namespace)::RequestProxy::NotifyReceivedData(int) webkit/tools/test_shell/simple_resource_loader_bridge.cc:380 #30 0x2b67d28 in base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int)>, void ()((anonymous namespace)::RequestProxy* const&, int const&)>::MakeItSo(base::internal::RunnableAdapter<void ((anonymous namespace)::RequestProxy::*)(int)>, (anonymous namespace)::RequestProxy* const&, int const&) ./base/bind_internal.h:899 #31 0xa38de3 in MessageLoop::RunTask(base::PendingTask const&) base/message_loop.cc:457 #32 0xa395cd in MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) base/message_loop.cc:468 #33 0xa39aa2 in MessageLoop::DoWork() base/message_loop.cc:644 #34 0xa90c05 in base::MessagePumpGlib::HandleDispatch() base/message_pump_glib.cc:268 #35 0xa8fd09 in (anonymous namespace)::WorkSourceDispatch(_GSource*, int (*)(void*), void*) base/message_pump_glib.cc:105 #36 0x7f5360d738c2 in g_main_dispatch /build/buildd/glib2.0-2.24.1/glib/gmain.c:1960 Stats: 13M malloced (22M for red zones) by 74836 calls Stats: 1M realloced by 1420 calls Stats: 11M freed by 62518 calls Stats: 0M really freed by 0 calls Stats: 68M (17417 full pages) mmaped in 17 calls mmaps by size class: 8:81915; 9:8191; 10:4095; 11:2047; 12:1024; 13:512; 14:256; 15:128; 16:64; 17:32; 18:32; 19:8; mallocs by size class: 8:69435; 9:2413; 10:1498; 11:724; 12:314; 13:147; 14:193; 15:73; 16:14; 17:5; 18:18; 19:2; frees by size class: 8:57942; 9:2045; 10:1239; 11:609; 12:281; 13:127; 14:179; 15:63; 16:9; 17:4; 18:18; 19:2; rfrees by size class: Stats: malloc large: 25 small slow: 260
ccing current Chromium webkit sheriff in case he is seeing crashes on this. but as i am thinking it will be a lot.
Created attachment 152798 [details] Patch Add checks for document destruction when attaching nodes
Comment on attachment 152798 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152798&action=review > Source/WebCore/ChangeLog:3 > + Unneeded tree walking when adding or removing children due to RenderCounter / RenderQuote logic Please fix the title. Like Regression(xyz): Crash in ... > Source/WebCore/ChangeLog:8 > + Check for document destruction during insertion to avoid crashes and also since it's just unneeded Please example the why part, like it causes null ptr crash since the view is null.
(In reply to comment #56) > (From update of attachment 152798 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152798&action=review > > > Source/WebCore/ChangeLog:3 > > + Unneeded tree walking when adding or removing children due to RenderCounter / RenderQuote logic > > Please fix the title. Like Regression(xyz): Crash in ... Should I also rename the actual bug?
(In reply to comment #57) > (In reply to comment #56) > > (From update of attachment 152798 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=152798&action=review > > > > > Source/WebCore/ChangeLog:3 > > > + Unneeded tree walking when adding or removing children due to RenderCounter / RenderQuote logic > > > > Please fix the title. Like Regression(xyz): Crash in ... > > Should I also rename the actual bug? No, it is not needed. Ideally, i should have filed a new bug, which i didn't do.
Comment on attachment 152798 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152798&action=review >>>> Source/WebCore/ChangeLog:3 >>>> + Unneeded tree walking when adding or removing children due to RenderCounter / RenderQuote logic >>> >>> Please fix the title. Like Regression(xyz): Crash in ... >> >> Should I also rename the actual bug? > > No, it is not needed. Ideally, i should have filed a new bug, which i didn't do. We prefer to have one patch per bug. This policy keeps the bug comments on bugzilla shorter thus reduces the signal-to-noise ratio when looking at older discussions. Please file a follow-up bug for this patch.
It would have been better if you guys filed a new bug for the regression.
(In reply to comment #60) > It would have been better if you guys filed a new bug for the regression. Yeah, new bug is filed as https://bugs.webkit.org/show_bug.cgi?id=91547.