| Summary: | Removing an element from an anonymous block causes crash | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Guillory <ctguil> | ||||||||||
| Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, bashi, cfleizach, ddkilzer, dglazkov, dmazzoni, eric, hamaji, hyatt, levin, mitz, mjs, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Chris Guillory
2010-07-14 18:03:53 PDT
thanks for creating a reproducible case. i'll take a look tomorrow AccessibilityRenderObject::nextSibling is returning an element, but then AccessibilityObject* AccessibilityRenderObject::parentObjectIfExists() const is returning a different element 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 Could we change the code so that each AccessibleObject keeps a pointer to its parent? (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 Created attachment 61981 [details]
Patch
WebCore/accessibility code looks good to me. Someone else should over the WebCore/rendering changes. hyatt, mitz -- pretty please? Created attachment 62708 [details]
Patch
adding new patch which is smaller than original. i've gotten rid of all the changes that are not directly related to this bug anyone want to review this? > 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.
(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. > 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.
(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 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.
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. 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. :) (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 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, Created attachment 64034 [details]
Patch
(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 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. Comment on attachment 64034 [details]
Patch
r=me
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 |