Bug 8952 – REGRESSION: crash on drag of highlighted Google custom home page modules

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[ABob]

Safari WebKit r14382 crashes when moving module between divs

Steps:
0. Use WebKit 14382 to open http://www.google.com/ig
1. Drag select various cells, so that they become highlighted
2. Drag and drop the selected module across column divisions,
3. Repeat 1-3 x's until Safari Web Kit crashes.

Result:
Safari WebKit crashes
Please see the attached crash log and screen shot.

OS X Regression:
   Problem does not occur in FireFox 1.5.0.2
   Problem does not occur in Safari 2.0.3

[ABob]

Created attachment 8362 [details]
Crash log

Confirmed. I think a reduction would be nice.

This is a case of Document::updateRendering() being called under detach():

#0  WebCore::RenderCanvas::setSelection (this=0x260018fc, s=0x0, sp=-1, e=0x0, ep=-1) at WebCore/rendering/RenderCanvas.cpp:327
#1  0x018b459b in WebCore::RenderCanvas::clearSelection (this=0x260018fc) at WebCore/rendering/RenderCanvas.cpp:452
#2  0x018b4eb6 in WebCore::RenderContainer::removeChildNode (this=0x25a7ce2c, oldChild=0x25a7cedc) at WebCore/rendering/RenderContainer.cpp:180
#3  0x018b506f in WebCore::RenderContainer::removeChild (this=0x25a7ce2c, oldChild=0x25a7cedc) at WebCore/rendering/RenderContainer.cpp:213
#4  0x0189c04f in WebCore::RenderBlock::removeChild (this=0x25a7ce2c, oldChild=0x25a7cedc) at WebCore/rendering/RenderBlock.cpp:318
#5  0x018ce4db in WebCore::RenderObject::remove (this=0x25a7cedc) at WebCore/rendering/RenderObject.cpp:2051
#6  0x018ce5e7 in WebCore::RenderObject::destroy (this=0x25a7cedc) at WebCore/rendering/RenderObject.cpp:2067
#7  0x018dcba3 in WebCore::RenderText::destroy (this=0x25a7cedc) at WebCore/rendering/RenderText.cpp:143
#8  0x01979682 in WebCore::Node::detach (this=0x25a7cd80) at WebCore/dom/Node.cpp:712
#9  0x018334e3 in WebCore::ContainerNode::detach (this=0x25a7db30) at WebCore/dom/ContainerNode.cpp:582
#10 0x018334e3 in WebCore::ContainerNode::detach (this=0x25a95940) at WebCore/dom/ContainerNode.cpp:582
#11 0x018334e3 in WebCore::ContainerNode::detach (this=0x25af8830) at WebCore/dom/ContainerNode.cpp:582
#12 0x018334e3 in WebCore::ContainerNode::detach (this=0x26001c50) at WebCore/dom/ContainerNode.cpp:582
#13 0x018334e3 in WebCore::ContainerNode::detach (this=0x25afd640) at WebCore/dom/ContainerNode.cpp:582
#14 0x018334e3 in WebCore::ContainerNode::detach (this=0x25afe4b0) at WebCore/dom/ContainerNode.cpp:582
#15 0x0197f04e in WebCore::Element::recalcStyle (this=0x25afe4b0, change=NoChange) at WebCore/dom/Element.cpp:508
#16 0x0197f250 in WebCore::Element::recalcStyle (this=0x26000190, change=NoChange) at WebCore/dom/Element.cpp:541
#17 0x0197f250 in WebCore::Element::recalcStyle (this=0x26091a60, change=NoChange) at WebCore/dom/Element.cpp:541
#18 0x0197f250 in WebCore::Element::recalcStyle (this=0x25ebf010, change=NoChange) at WebCore/dom/Element.cpp:541
#19 0x0197f250 in WebCore::Element::recalcStyle (this=0x25eb2020, change=NoChange) at WebCore/dom/Element.cpp:541
#20 0x0197f250 in WebCore::Element::recalcStyle (this=0x25ea1810, change=NoChange) at WebCore/dom/Element.cpp:541
#21 0x0197f250 in WebCore::Element::recalcStyle (this=0x25ead450, change=NoChange) at WebCore/dom/Element.cpp:541
#22 0x0197f250 in WebCore::Element::recalcStyle (this=0x260ae9a0, change=NoChange) at WebCore/dom/Element.cpp:541
#23 0x0182c7d2 in WebCore::Document::recalcStyle (this=0x11958200, change=NoChange) at WebCore/dom/Document.cpp:846
#24 0x01825e47 in WebCore::Document::updateRendering (this=0x11958200) at WebCore/dom/Document.cpp:868
#25 0x01739eeb in WebCore::HTMLElement::isContentEditable (this=0x25a7db30) at WebCore/html/HTMLElement.cpp:443
#26 0x01978726 in WebCore::Node::isContentEditable (this=0x25a7cd80) at WebCore/dom/Node.cpp:320
#27 0x0197a294 in WebCore::Node::rootEditableElement (this=0x25a7cd80) at WebCore/dom/Node.cpp:1047
#28 0x0189e399 in WebCore::RenderBlock::isSelectionRoot (this=0x25a7ce2c) at WebCore/rendering/RenderBlock.cpp:1461
#29 0x0189e40e in WebCore::RenderBlock::shouldPaintSelectionGaps (this=0x25a7ce2c) at WebCore/rendering/RenderBlock.cpp:1443
#30 0x018a410d in WebCore::RenderBlock::selectionGapRects (this=0x25a7ce2c) at WebCore/rendering/RenderBlock.cpp:1470
#31 0x01ab49dc in WebCore::RenderBlock::BlockSelectionInfo::BlockSelectionInfo (this=0x260df270, b=0x25a7ce2c) at WebCore/rendering/RenderBlock.h:236
#32 0x018b3581 in WebCore::RenderCanvas::setSelection (this=0x260018fc, s=0x0, sp=-1, e=0x0, ep=-1) at WebCore/rendering/RenderCanvas.cpp:337
#33 0x018b459b in WebCore::RenderCanvas::clearSelection (this=0x260018fc) at WebCore/rendering/RenderCanvas.cpp:452
#34 0x018b4eb6 in WebCore::RenderContainer::removeChildNode (this=0x25a7ce2c, oldChild=0x25a7cedc) at WebCore/rendering/RenderContainer.cpp:180
#35 0x018b506f in WebCore::RenderContainer::removeChild (this=0x25a7ce2c, oldChild=0x25a7cedc) at WebCore/rendering/RenderContainer.cpp:213
#36 0x0189c04f in WebCore::RenderBlock::removeChild (this=0x25a7ce2c, oldChild=0x25a7cedc) at WebCore/rendering/RenderBlock.cpp:318
#37 0x018ce4db in WebCore::RenderObject::remove (this=0x25a7cedc) at WebCore/rendering/RenderObject.cpp:2051
#38 0x018ce5e7 in WebCore::RenderObject::destroy (this=0x25a7cedc) at WebCore/rendering/RenderObject.cpp:2067
#39 0x018dcba3 in WebCore::RenderText::destroy (this=0x25a7cedc) at WebCore/rendering/RenderText.cpp:143
#40 0x01979682 in WebCore::Node::detach (this=0x25a7cd80) at WebCore/dom/Node.cpp:712
#41 0x018334e3 in WebCore::ContainerNode::detach (this=0x25a7db30) at WebCore/dom/ContainerNode.cpp:582
#42 0x018334e3 in WebCore::ContainerNode::detach (this=0x25a95940) at WebCore/dom/ContainerNode.cpp:582

You even end up re-entering setSelection().

[ABob]

The key to this bug is ** highlighted modules ** being dragged. 
The problem does not happen if the modules are not highlighted.

Created attachment 8417 [details]
Reduction (will crash)

[Darin Adler]

SelectionController::nodeWillBeRemoved does take care of this same issue, so the clearSelection call in RenderContainer::removeChildNode could perhaps be removed entirely to fix this bug.

[Alice Liu]

<rdar://problem/4575185>

*** Bug 9425 has been marked as a duplicate of this bug. ***

[Joost de Valk (AlthA)]

Adding GoogleBug keyword in one big change.

[Darin Adler]

Created attachment 9423 [details]
patch that fixes the bug -- needs change log, layout test, etc.

[Bob Austin]

Regressed the problem with WebKit 15403 on 10.4.6.
Problem continues with the same steps to reproduce as well as the attached regression.
Please see attached log. It's slightly different than the original crash log.

I'll also update this problem with a Safari 2.0.4 regression on 10.4.7.

[Darin Adler]

Created attachment 9445 [details]
patch with detailed change log and a layout test

[Justin Garcia]

Now we'd blow away the selection if a node inside it is removed, instead of just invalidating the rect(s) that it paints in.  Is that necessary to fix the crash?

[Darin Adler]

(In reply to comment #13)
> Now we'd blow away the selection if a node inside it is removed, instead of
> just invalidating the rect(s) that it paints in.  Is that necessary to fix the
> crash?

I think you're misreading the patch. I didn't change that.

[Darin Adler]

(In reply to comment #13)
> Now we'd blow away the selection if a node inside it is removed, instead of
> just invalidating the rect(s) that it paints in.  Is that necessary to fix the
> crash?

Here's the old code that does it:

-    } else if (Range::compareBoundaryPoints(m_sel.start(), Position(node, 0)) == -1 &&
-               Range::compareBoundaryPoints(m_sel.end(), Position(node, 0)) == 1) {
-        // If we did nothing here, when this node's renderer was destroyed, the rect that it 
-        // occupied would be invalidated, but, selection gaps that change as a result of 
-        // the removal wouldn't be invalidated.
-        // FIXME: Don't do so much unnecessary invalidation.
-        static_cast<RenderView*>(start->document()->renderer())->clearSelection();

My new code does the same thing in this case.

[John Sullivan]

Comment on attachment 9445 [details]
patch with detailed change log and a layout test

r=me

[Darin Adler]

Committed revision 15459.

[Darin Adler]

Regression testing caught one small thing wrong with the change. Changed logic back to what it was before, but retained the bug fix.

Committed revision 15461.