Bug 8952 - REGRESSION: crash on drag of highlighted Google custom home page modules
Summary: REGRESSION: crash on drag of highlighted Google custom home page modules
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Critical
Assignee: Darin Adler
URL: http://www.google.com/ig
Keywords: GoogleBug, HasReduction, InRadar, Regression
: 9425 (view as bug list)
Depends on:
Blocks: 8370
  Show dependency treegraph
 
Reported: 2006-05-17 00:13 PDT by ABob
Modified: 2006-07-15 13:59 PDT (History)
7 users (show)

See Also:


Attachments
Crash log (25.12 KB, text/plain)
2006-05-17 00:18 PDT, ABob
no flags Details
Reduction (will crash) (326 bytes, text/html)
2006-05-19 10:41 PDT, mitz
no flags Details
patch that fixes the bug -- needs change log, layout test, etc. (1000 bytes, patch)
2006-07-12 23:54 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch with detailed change log and a layout test (14.01 KB, patch)
2006-07-14 06:01 PDT, Darin Adler
sullivan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ABob 2006-05-17 00:13:48 PDT
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
Comment 1 ABob 2006-05-17 00:18:25 PDT
Created attachment 8362 [details]
Crash log
Comment 2 jonathanjohnsson 2006-05-17 00:25:13 PDT
Confirmed. I think a reduction would be nice.
Comment 3 mitz 2006-05-17 13:41:04 PDT
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().
Comment 4 ABob 2006-05-18 22:14:38 PDT
The key to this bug is ** highlighted modules ** being dragged. 
The problem does not happen if the modules are not highlighted.
Comment 5 mitz 2006-05-19 10:41:40 PDT
Created attachment 8417 [details]
Reduction (will crash)
Comment 6 Darin Adler 2006-06-04 17:05:49 PDT
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.
Comment 7 Alice Liu 2006-06-06 09:07:18 PDT
<rdar://problem/4575185>
Comment 8 mitz 2006-06-13 12:57:30 PDT
*** Bug 9425 has been marked as a duplicate of this bug. ***
Comment 9 Joost de Valk (AlthA) 2006-07-12 10:20:38 PDT
Adding GoogleBug keyword in one big change.
Comment 10 Darin Adler 2006-07-12 23:54:11 PDT
Created attachment 9423 [details]
patch that fixes the bug -- needs change log, layout test, etc.
Comment 11 Bob Austin 2006-07-13 01:40:54 PDT
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.
Comment 12 Darin Adler 2006-07-14 06:01:11 PDT
Created attachment 9445 [details]
patch with detailed change log and a layout test
Comment 13 Justin Garcia 2006-07-14 12:22:39 PDT
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?
Comment 14 Darin Adler 2006-07-14 12:58:44 PDT
(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.
Comment 15 Darin Adler 2006-07-14 13:00:08 PDT
(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.
Comment 16 John Sullivan 2006-07-15 09:20:44 PDT
Comment on attachment 9445 [details]
patch with detailed change log and a layout test

r=me
Comment 17 Darin Adler 2006-07-15 12:36:00 PDT
Committed revision 15459.
Comment 18 Darin Adler 2006-07-15 13:59:11 PDT
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.