Bug 89900 – Unneeded tree walking when adding or removing children due to RenderCounter / RenderQuote logic

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Abhishek Arya]

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;
        }
}

[Julien Chaffraix]

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.

[Elliott Sprehn]

Maybe we can put a flag or counter in the RenderView to turn this on and off?

[Julien Chaffraix]

(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).

[Elliott Sprehn]

Created attachment 150452 [details]
Patch

Track the number of RenderQuote and RenderCounter in the RenderView

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 150452 [details]
Patch

Attachment 150452 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13127254

[WebKit Review Bot]

Comment on attachment 150452 [details]
Patch

Attachment 150452 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13135121

[Early Warning System Bot]

Comment on attachment 150452 [details]
Patch

Attachment 150452 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13124232

[Tony Chang]

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.

[Gyuyoung Kim]

Comment on attachment 150452 [details]
Patch

Attachment 150452 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13126243

[Elliott Sprehn]

Created attachment 150454 [details]
Patch

Fix busted compile...

[Elliott Sprehn]

(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.

[Eric Seidel (no email)]

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?

[Abhishek Arya]

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"

[Ryosuke Niwa]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Elliott Sprehn]

Created attachment 150476 [details]
Patch

Different approach that uses willBeDestroyed and adds document destruction checks for cases where the RenderView may not exist.

[Elliott Sprehn]

(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.

[Abhishek Arya]

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.

[Elliott Sprehn]

Created attachment 150490 [details]
Patch

Fix review comments

[Elliott Sprehn]

(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.

[Ojan Vafai]

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?

[Abhishek Arya]

(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.

[Abhishek Arya]

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.

[Ojan Vafai]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Elliott Sprehn]

Created attachment 150502 [details]
Patch

Clean up ChangeLog.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Elliott Sprehn]

Created attachment 150511 [details]
Patch

Avoid accessing view() in willBeDestroyed() when the document itself is being destroyed

[Elliott Sprehn]

(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.

[Abhishek Arya]

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.

[Carol Szabo]

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.

[Abhishek Arya]

(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

[Carol Szabo]

(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.

[Elliott Sprehn]

Created attachment 152047 [details]
Patch

Updated with comments from inferno

[Eric Seidel (no email)]

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.

[Elliott Sprehn]

(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?

[Ojan Vafai]

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.

[Eric Seidel (no email)]

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.

[Elliott Sprehn]

Created attachment 152053 [details]
Patch

Updated with comments from Eric Sidel

[Elliott Sprehn]

(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

[Elliott Sprehn]

(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.

[Eric Seidel (no email)]

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!)

[Eric Seidel (no email)]

Comment on attachment 152053 [details]
Patch

I like this patch.  Thanks.

[Elliott Sprehn]

Created attachment 152061 [details]
Patch for landing

[Elliott Sprehn]

Created attachment 152077 [details]
Patch for landing

[Julien Chaffraix]

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).

[WebKit Review Bot]

Comment on attachment 152077 [details]
Patch for landing

Clearing flags on attachment: 152077

Committed r122524: <http://trac.webkit.org/changeset/122524>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Abhishek Arya]

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

[Abhishek Arya]

ccing current Chromium webkit sheriff in case he is seeing crashes on this. but as i am thinking it will be a lot.

[Elliott Sprehn]

Created attachment 152798 [details]
Patch

Add checks for document destruction when attaching nodes

[Abhishek Arya]

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.

[Elliott Sprehn]

(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?

[Abhishek Arya]

(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.

[Julien Chaffraix]

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.

[Ryosuke Niwa]

It would have been better if you guys filed a new bug for the regression.

[Abhishek Arya]

(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.