Bug 42309 – Removing an element from an anonymous block causes crash

• Component: Accessibility
• Status: RESOLVED FIXED

[Chris Guillory]

Created attachment 61593 [details]
Layout test that reproduces crash within the chromium testshell

This bug is causing the chromium renderer to crash.
http://code.google.com/p/chromium/issues/detail?id=47126

After much debugging I have a layout test that reproduces the crash within the chromium testshell. This appears to be a similar issue to https://bugs.webkit.org/show_bug.cgi?id=41000.

[chris fleizach]

thanks for creating a reproducible case. i'll take a look tomorrow

[chris fleizach]

AccessibilityRenderObject::nextSibling is returning an element, but then 

AccessibilityObject* AccessibilityRenderObject::parentObjectIfExists() const

is returning a different element

[chris fleizach]

So this is a regression from the change to support continuations. The problem is that a child is being added (because its sibling was a continuation). However, it reports its parent as a different object.

I have a fix that changes what's returned for the parentObject(). The change adds a 3rd case to check. That 3rd case is if the first sibling is part of a continuation, then following the continuation back to its origin. Then take the parent. That parent is the same parent that "found" this element and added it as its child.

To accomplish, however, I had to store the "continuator" when setting a continuation on a RenderBlock/Inline. I couldn't find any other way to go backwards from element that WAS a continuation. It seemed all the code was designed only to FOLLOW continuations.

I've also been able to add some ASSERTs that will help expose more problems like this right away.

Will test this change more and add a patch soon

[Chris Guillory]

Could we change the code so that each AccessibleObject keeps a pointer to its parent?

[chris fleizach]

(In reply to comment #4)
> Could we change the code so that each AccessibleObject keeps a pointer to its parent?

we still have the requirement to go from an arbitrary RenderObject -> AccessibilityObject, whose parent may not have been created yet. if its parent object was asked for, we'd have to be able to retrieve the right ax parent still

[chris fleizach]

Created attachment 61981 [details]
Patch

[Chris Guillory]

WebCore/accessibility code looks good to me. Someone else should over the WebCore/rendering changes.

[Dimitri Glazkov (Google)]

hyatt, mitz -- pretty please?

[chris fleizach]

Created attachment 62708 [details]
Patch

[chris fleizach]

adding new patch which is smaller than original. i've gotten rid of all the changes that are not directly related to this bug

[chris fleizach]

anyone want to review this?

[Shinichiro Hamaji]

> To accomplish, however, I had to store the "continuator" when setting a continuation on a RenderBlock/Inline. I couldn't find any other way to go backwards from element that WAS a continuation. It seemed all the code was designed only to FOLLOW continuations.

Have you already considered the following two approaches?

Is AccessibilityRenderObject::renderParentObject() called often? If performance of this function isn't so important, cannot we iterate the render tree in reverse order to find the continuator using the forward links?

Another way to avoid increasing memory consumption would be hiring ugly idiom which uses global HashMap. Please check global hashmaps in RenderBlock.cpp for example, in case you aren't familiar with the hack.

[chris fleizach]

(In reply to comment #12)
> > To accomplish, however, I had to store the "continuator" when setting a continuation on a RenderBlock/Inline. I couldn't find any other way to go backwards from element that WAS a continuation. It seemed all the code was designed only to FOLLOW continuations.
> 
> Have you already considered the following two approaches?
> 
> Is AccessibilityRenderObject::renderParentObject() called often? If performance of this function isn't so important, cannot we iterate the render tree in reverse order to find the continuator using the forward links?

performance is a huge issue here and this method can be called 1000s of times a second

> 
> Another way to avoid increasing memory consumption would be hiring ugly idiom which uses global HashMap. Please check global hashmaps in RenderBlock.cpp for example, in case you aren't familiar with the hack.

i don't see a need to use an "ugly idiom" when this approach is working just fine, and requires less memory ostensibly, than a HashMap full of elements that needs to be accessed early and often.

Honestly, i don't know whats going on with this patch. in one form or another its been ready for 3 weeks. no one has even left any comments despite my sending direct emails to people and CCing half of webkit.

[Shinichiro Hamaji]

> i don't see a need to use an "ugly idiom" when this approach is working just fine, and requires less memory ostensibly, than a HashMap full of elements that needs to be accessed early and often.

I see. I guessed m_continuator will be 0 for most cases so we can save some amount of memory, but I might be wrong. Sorry for confusion.

[chris fleizach]

(In reply to comment #14)
> > i don't see a need to use an "ugly idiom" when this approach is working just fine, and requires less memory ostensibly, than a HashMap full of elements that needs to be accessed early and often.
> 
> I see. I guessed m_continuator will be 0 for most cases so we can save some amount of memory, but I might be wrong. Sorry for confusion.

Of course if there is some other way to go back the continuation chain that I don't know about, I'd be happy to use that method

[Dave Hyatt]

Comment on attachment 62708 [details]
Patch

No to adding  4 bytes to all blocks and inlines.  You can just do what all the other code does that has to find the previous continuation... walk the chain from the beginning.  Continuation chains are rarely long in practice, so this is not a big deal.

[Dave Hyatt]

Check out continuationBefore if you need to see code that does this.  You can basically use the DOM node's renderer as a starting point and walk the continuation chain to get to the one you want.  You don't have to iterate through the render tree sibling pointers.

[Dave Hyatt]

Continuation pointers are definitely a good candidate to be placed in HashMaps, since they are 0 nearly all the time.  Having forward and backward connections(In reply to comment #14)
> > i don't see a need to use an "ugly idiom" when this approach is working just fine, and requires less memory ostensibly, than a HashMap full of elements that needs to be accessed early and often.
> 
> I see. I guessed m_continuator will be 0 for most cases so we can save some amount of memory, but I might be wrong. Sorry for confusion.

No, you are correct.  Even the existing forward continuation pointer is a good candidate for a HashMap conversion and would save a significant chunk of memory.  I'd review a patch that did that. :)

[chris fleizach]

(In reply to comment #17)
> Check out continuationBefore if you need to see code that does this.  You can basically use the DOM node's renderer as a starting point and walk the continuation chain to get to the one you want.  You don't have to iterate through the render tree sibling pointers.

Thanks for these pointers. I see that there are two (nearly) identical continuationBefore() methods in RenderInline and RenderBlock

would it make sense to merge these into one method so that AXRenderObject could also use it, or should AXRenderObject make its own copy.

I say, nearly identical, because when RenderBlock wants the next continuation it does

curr->continuation()

and RenderInline does

curr->nextContination() --- where

static RenderBoxModelObject* nextContinuation(RenderObject* renderer)
{
    if (renderer->isInline() && !renderer->isReplaced())
        return toRenderInline(renderer)->continuation();
    return toRenderBlock(renderer)->inlineElementContinuation();
}
------
but i assume those differences could be resolved

[Kenichi Ishibashi]

Hi,

(In reply to comment #18)
> Continuation pointers are definitely a good candidate to be placed in HashMaps, since they are 0 nearly all the time.  Having forward and backward connections(In reply to comment #14)

I'd like to make a patch to migrate m_continuation pointers to a HashMap and to merge similar functions on RenderInline and RenderBlock. I'll file an issue for that and send a patch soon.

Thanks,

[chris fleizach]

Created attachment 64034 [details]
Patch

[chris fleizach]

(In reply to comment #21)
> Created an attachment (id=64034) [details]
> Patch

new patch follow's Dave's suggestion. look at the node()->renderer() and then follow the continuation chain

[Chris Guillory]

With this change I am unable to reproduce the renderer crash in Chromium. Regarding the change, the new assert in addChildren will fire if a similar issue occurs so that's good. Someone with webkit experience should look over the continuation following code.

[Dave Hyatt]

Comment on attachment 64034 [details]
Patch

r=me

[chris fleizach]

http://trac.webkit.org/changeset/65095

[WebKit Review Bot]

http://trac.webkit.org/changeset/65095 might have broken SnowLeopard Intel Release (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/65094
http://trac.webkit.org/changeset/65095