This crash can be consistently reproduced at www.multiply.com with the following steps: 1. Create a Multiply account 2. Compose a new blog entry with some short text 3. Select a picture from disk to insert Crash.
Created attachment 69696 [details] Patch
Comment on attachment 69696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69696&action=review > WebCore/rendering/RenderWidget.cpp:-83 > - if (widgetHierarchyUpdateSuspendCount == 1) { > - WidgetToParentMap map = widgetNewParentMap(); > - widgetNewParentMap().clear(); > - WidgetToParentMap::iterator end = map.end(); > - for (WidgetToParentMap::iterator it = map.begin(); it != end; ++it) { > - Widget* child = it->first.get(); > - ScrollView* currentParent = child->parent(); > - FrameView* newParent = it->second; > - if (newParent != currentParent) { > - if (currentParent) > - currentParent->removeChild(child); > - if (newParent) > - newParent->addChild(child); > - } > + if (--widgetHierarchyUpdateSuspendCount) > + return; > + WidgetToParentMap map = widgetNewParentMap(); > + widgetNewParentMap().clear(); > + WidgetToParentMap::iterator end = map.end(); > + for (WidgetToParentMap::iterator it = map.begin(); it != end; ++it) { > + Widget* child = it->first.get(); > + ScrollView* currentParent = child->parent(); > + FrameView* newParent = it->second; > + if (newParent != currentParent) { > + if (currentParent) > + currentParent->removeChild(child); > + if (newParent) > + newParent->addChild(child); > } > } > - widgetHierarchyUpdateSuspendCount--; This change allows hierarchy updates to happen synchronously during this function. The code was written purposefully to disallow this, so I don’t think this change is right (in particular, it can cause earlier hierarchy changes to win over later ones). Hitting the assertion is often a sign of some other problem in the code. Are you still hitting it with the above change to canonicalPosition()?
Created attachment 69705 [details] patch2
(In reply to comment #2) > (From update of attachment 69696 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69696&action=review > > > WebCore/rendering/RenderWidget.cpp:-83 > > - if (widgetHierarchyUpdateSuspendCount == 1) { > > - WidgetToParentMap map = widgetNewParentMap(); > > - widgetNewParentMap().clear(); > > - WidgetToParentMap::iterator end = map.end(); > > - for (WidgetToParentMap::iterator it = map.begin(); it != end; ++it) { > > - Widget* child = it->first.get(); > > - ScrollView* currentParent = child->parent(); > > - FrameView* newParent = it->second; > > - if (newParent != currentParent) { > > - if (currentParent) > > - currentParent->removeChild(child); > > - if (newParent) > > - newParent->addChild(child); > > - } > > + if (--widgetHierarchyUpdateSuspendCount) > > + return; > > + WidgetToParentMap map = widgetNewParentMap(); > > + widgetNewParentMap().clear(); > > + WidgetToParentMap::iterator end = map.end(); > > + for (WidgetToParentMap::iterator it = map.begin(); it != end; ++it) { > > + Widget* child = it->first.get(); > > + ScrollView* currentParent = child->parent(); > > + FrameView* newParent = it->second; > > + if (newParent != currentParent) { > > + if (currentParent) > > + currentParent->removeChild(child); > > + if (newParent) > > + newParent->addChild(child); > > } > > } > > - widgetHierarchyUpdateSuspendCount--; > > This change allows hierarchy updates to happen synchronously during this function. The code was written purposefully to disallow this, so I don’t think this change is right (in particular, it can cause earlier hierarchy changes to win over later ones). Hitting the assertion is often a sign of some other problem in the code. Are you still hitting it with the above change to canonicalPosition()? Removed the change to RenderWidget.cpp. Created https://bugs.webkit.org/show_bug.cgi?id=47126 to track the assert issue.
Sorry, my fault, I pushed Enrica to make this change.
Comment on attachment 69705 [details] patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=69705&action=review > WebCore/editing/VisiblePosition.cpp:444 > + // The updateLayout call below can do so much, that even the position passed This comma should be removed. > LayoutTests/editing/selection/focus-crash.html:45 > \ No newline at end of file Why no newline?
Addressed comments. Committed revision 69051.