Crash at WebCore::nextCandidate + 27

• Component: HTML Editing
• Status: RESOLVED FIXED

[Enrica Casucci]

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.

[Enrica Casucci]

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

[Enrica Casucci]

Created attachment 69705 [details]
patch2

[Enrica Casucci]

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

[Darin Adler]

Sorry, my fault, I pushed Enrica to make this change.

[Darin Adler]

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?

[Enrica Casucci]

Addressed comments.
Committed revision 69051.