Bug 112525

Summary: AXObjectCache gets recreated during document tear-down
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: AccessibilityAssignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aboxhall, apinheiro, bdakin, buildbot, cfleizach, cmarcelo, dglazkov, dmazzoni, eric, esprehn+autocc, japhet, jdiggs, mifenton, ojan.autocc, peter+ews, rniwa, simon.fraser, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
abarth: review-
patch
none
patch
simon.fraser: review+, webkit.review.bot: commit-queue-
patch
webkit.review.bot: commit-queue-
patch
webkit.review.bot: commit-queue-
patch
webkit.review.bot: commit-queue-
patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-07
none
patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-03
none
patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-03
none
patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-01
none
Archive of layout-test-results from gce-cr-linux-04
none
patch none

Description Simon Fraser (smfr) 2013-03-17 16:22:05 PDT
Was debugging LayoutTests/accessibility/accessibility-node-reparent.html in DRT.

Document::detach() does:
    if (this == topDocument())
        clearAXObjectCache();

[Aside: why doesn't it unconditionally clear it? Only the top document should have one, so if this isn't the top document, it shouldn't have had one anyway]

A few lines down, it then does:
    ContainerNode::detach();
which makes a new AXObjectCache

    frame #0: 0x0000000101aa59f2 WebCore`WebCore::AXObjectCache::AXObjectCache(WebCore::Document const*) + 210 at AXObjectCache.cpp:110
    frame #1: 0x0000000101aa590d WebCore`WebCore::AXObjectCache::AXObjectCache(WebCore::Document const*) + 29 at AXObjectCache.cpp:111
    frame #2: 0x0000000101f2efd1 WebCore`WebCore::Document::axObjectCache() const + 209 at Document.cpp:2225
    frame #3: 0x000000010305f92b WebCore`WebCore::RenderObject::willBeDestroyed() + 283 at RenderObject.cpp:2421
    frame #4: 0x000000010301e6a2 WebCore`WebCore::RenderLayerModelObject::willBeDestroyed() + 162 at RenderLayerModelObject.cpp:90
    frame #5: 0x0000000102f420c9 WebCore`WebCore::RenderBoxModelObject::willBeDestroyed() + 153 at RenderBoxModelObject.cpp:339
    frame #6: 0x0000000102f1b6cd WebCore`WebCore::RenderBox::willBeDestroyed() + 61 at RenderBox.cpp:167
    frame #7: 0x0000000102e94042 WebCore`WebCore::RenderBlock::willBeDestroyed() + 578 at RenderBlock.cpp:303
    frame #8: 0x00000001030600dd WebCore`WebCore::RenderObject::destroy() + 29 at RenderObject.cpp:2575
    frame #9: 0x000000010305fff6 WebCore`WebCore::RenderObject::destroyAndCleanupAnonymousWrappers() + 54 at RenderObject.cpp:2553
    frame #10: 0x0000000102da5bf5 WebCore`WebCore::Node::detach() + 149 at Node.cpp:1114
    frame #11: 0x0000000101cf01ab WebCore`WebCore::ContainerNode::detach() + 43 at ContainerNode.cpp:834
    frame #12: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
    frame #13: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
    frame #14: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
    frame #15: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
    frame #16: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
    frame #17: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
    frame #18: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
    frame #19: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
    frame #20: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
    frame #21: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
    frame #22: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
    frame #23: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
    frame #24: 0x0000000101f2e9fd WebCore`WebCore::Document::detach() + 637 at Document.cpp:2152

Maybe this explains the crashes (bug 112523).
Comment 1 chris fleizach 2013-03-17 23:03:03 PDT
(In reply to comment #0)
> Was debugging LayoutTests/accessibility/accessibility-node-reparent.html in DRT.
> 
> Document::detach() does:
>     if (this == topDocument())
>         clearAXObjectCache();
> 
> [Aside: why doesn't it unconditionally clear it? Only the top document should have one, so if this isn't the top document, it shouldn't have had one anyway]
> 

It looks like clearAXObjectCache() does

topDocument()->m_axObjectCache.release();

So we wouldn't want to do that for every document that is detached.

> A few lines down, it then does:
>     ContainerNode::detach();
> which makes a new AXObjectCache
> 
>     frame #0: 0x0000000101aa59f2 WebCore`WebCore::AXObjectCache::AXObjectCache(WebCore::Document const*) + 210 at AXObjectCache.cpp:110
>     frame #1: 0x0000000101aa590d WebCore`WebCore::AXObjectCache::AXObjectCache(WebCore::Document const*) + 29 at AXObjectCache.cpp:111
>     frame #2: 0x0000000101f2efd1 WebCore`WebCore::Document::axObjectCache() const + 209 at Document.cpp:2225
>     frame #3: 0x000000010305f92b WebCore`WebCore::RenderObject::willBeDestroyed() + 283 at RenderObject.cpp:2421
>     frame #4: 0x000000010301e6a2 WebCore`WebCore::RenderLayerModelObject::willBeDestroyed() + 162 at RenderLayerModelObject.cpp:90
>     frame #5: 0x0000000102f420c9 WebCore`WebCore::RenderBoxModelObject::willBeDestroyed() + 153 at RenderBoxModelObject.cpp:339
>     frame #6: 0x0000000102f1b6cd WebCore`WebCore::RenderBox::willBeDestroyed() + 61 at RenderBox.cpp:167
>     frame #7: 0x0000000102e94042 WebCore`WebCore::RenderBlock::willBeDestroyed() + 578 at RenderBlock.cpp:303
>     frame #8: 0x00000001030600dd WebCore`WebCore::RenderObject::destroy() + 29 at RenderObject.cpp:2575
>     frame #9: 0x000000010305fff6 WebCore`WebCore::RenderObject::destroyAndCleanupAnonymousWrappers() + 54 at RenderObject.cpp:2553
>     frame #10: 0x0000000102da5bf5 WebCore`WebCore::Node::detach() + 149 at Node.cpp:1114
>     frame #11: 0x0000000101cf01ab WebCore`WebCore::ContainerNode::detach() + 43 at ContainerNode.cpp:834
>     frame #12: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
>     frame #13: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
>     frame #14: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
>     frame #15: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
>     frame #16: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
>     frame #17: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
>     frame #18: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
>     frame #19: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
>     frame #20: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
>     frame #21: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
>     frame #22: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
>     frame #23: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
>     frame #24: 0x0000000101f2e9fd WebCore`WebCore::Document::detach() + 637 at Document.cpp:2152
> 
> Maybe this explains the crashes (bug 112523).

(In reply to comment #0)
> Was debugging LayoutTests/accessibility/accessibility-node-reparent.html in DRT.
> 
> Document::detach() does:
>     if (this == topDocument())
>         clearAXObjectCache();
> 
> [Aside: why doesn't it unconditionally clear it? Only the top document should have one, so if this isn't the top document, it shouldn't have had one anyway]
> 
> A few lines down, it then does:
>     ContainerNode::detach();
> which makes a new AXObjectCache
> 
>     frame #0: 0x0000000101aa59f2 WebCore`WebCore::AXObjectCache::AXObjectCache(WebCore::Document const*) + 210 at AXObjectCache.cpp:110
>     frame #1: 0x0000000101aa590d WebCore`WebCore::AXObjectCache::AXObjectCache(WebCore::Document const*) + 29 at AXObjectCache.cpp:111
>     frame #2: 0x0000000101f2efd1 WebCore`WebCore::Document::axObjectCache() const + 209 at Document.cpp:2225
>     frame #3: 0x000000010305f92b WebCore`WebCore::RenderObject::willBeDestroyed() + 283 at RenderObject.cpp:2421
>     frame #4: 0x000000010301e6a2 WebCore`WebCore::RenderLayerModelObject::willBeDestroyed() + 162 at RenderLayerModelObject.cpp:90
>     frame #5: 0x0000000102f420c9 WebCore`WebCore::RenderBoxModelObject::willBeDestroyed() + 153 at RenderBoxModelObject.cpp:339
>     frame #6: 0x0000000102f1b6cd WebCore`WebCore::RenderBox::willBeDestroyed() + 61 at RenderBox.cpp:167
>     frame #7: 0x0000000102e94042 WebCore`WebCore::RenderBlock::willBeDestroyed() + 578 at RenderBlock.cpp:303
>     frame #8: 0x00000001030600dd WebCore`WebCore::RenderObject::destroy() + 29 at RenderObject.cpp:2575
>     frame #9: 0x000000010305fff6 WebCore`WebCore::RenderObject::destroyAndCleanupAnonymousWrappers() + 54 at RenderObject.cpp:2553
>     frame #10: 0x0000000102da5bf5 WebCore`WebCore::Node::detach() + 149 at Node.cpp:1114
>     frame #11: 0x0000000101cf01ab WebCore`WebCore::ContainerNode::detach() + 43 at ContainerNode.cpp:834
>     frame #12: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
>     frame #13: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
>     frame #14: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
>     frame #15: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
>     frame #16: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
>     frame #17: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
>     frame #18: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
>     frame #19: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
>     frame #20: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
>     frame #21: 0x0000000102145f40 WebCore`WebCore::Element::detach() + 288 at Element.cpp:1310
>     frame #22: 0x0000000101cf2e07 WebCore`WebCore::ContainerNode::detachChildren() + 55 at ContainerNode.h:219
>     frame #23: 0x0000000101cf0199 WebCore`WebCore::ContainerNode::detach() + 25 at ContainerNode.cpp:832
>     frame #24: 0x0000000101f2e9fd WebCore`WebCore::Document::detach() + 637 at Document.cpp:2152
> 
> Maybe this explains the crashes (bug 112523).
Comment 2 Simon Fraser (smfr) 2013-03-18 08:34:14 PDT
And here's a hang under the accessibility notification firing:

http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r146065%20(7868)/svg/wicd/sizing-flakiness-sample.txt
Comment 3 chris fleizach 2013-03-18 10:17:49 PDT
Created attachment 193595 [details]
patch
Comment 4 WebKit Review Bot 2013-03-18 10:20:24 PDT
Attachment 193595 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/editing/DeleteFromTextNodeCommand.cpp', u'Source/WebCore/editing/Editor.cpp', u'Source/WebCore/editing/InsertIntoTextNodeCommand.cpp', u'Source/WebCore/editing/InsertNodeBeforeCommand.cpp', u'Source/WebCore/editing/mac/FrameSelectionMac.mm', u'Source/WebCore/html/HTMLInputElement.cpp', u'Source/WebCore/html/HTMLSelectElement.cpp', u'Source/WebCore/html/HTMLTextFormControlElement.cpp', u'Source/WebCore/html/InputType.cpp', u'Source/WebCore/html/RangeInputType.cpp', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/platform/ScrollView.cpp', u'Source/WebCore/platform/Scrollbar.cpp', u'Source/WebCore/platform/Scrollbar.h', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderListBox.cpp', u'Source/WebCore/rendering/RenderMenuList.cpp', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObjectChildList.cpp', u'Source/WebCore/rendering/RenderWidget.cpp']" exit_code: 1
Source/WebCore/platform/Scrollbar.h:178:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Adam Barth 2013-03-18 10:21:17 PDT
Comment on attachment 193595 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193595&action=review

> Source/WebCore/dom/Document.cpp:2214
> +    if (!topDocument()->renderer())
> +        return false;

This doesn't seem like the right thing to check here.
Comment 6 chris fleizach 2013-03-18 10:21:35 PDT
Created attachment 193597 [details]
patch
Comment 7 Adam Barth 2013-03-18 10:22:23 PDT
Comment on attachment 193597 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193597&action=review

> Source/WebCore/dom/Document.cpp:2214
> +    if (!topDocument()->renderer())
> +        return false;

Same comment as before.  I think you want to check whether the Document is detached from the Frame.
Comment 8 Simon Fraser (smfr) 2013-03-18 10:32:12 PDT
Comment on attachment 193597 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193597&action=review

> Source/WebCore/dom/Document.cpp:2209
>      // Clear the cache member variable before calling delete because attempts
>      // are made to access it during destruction.
> -    topDocument()->m_axObjectCache.release();
> +    topDocument()->m_axObjectCache.clear();
>  }

Can we rename this entire function, or never call it on the non-top document? It's really confusing that it can clear the cache on a doc that may not be this one.

> Source/WebCore/dom/Document.cpp:2211
>  bool Document::axObjectCacheExists() const

Maybe rename/change this one too.

> Source/WebCore/dom/Element.cpp:879
> +    if (AXObjectCache::accessibilityEnabled() && document()->axObjectCacheExists())
>          document()->axObjectCache()->handleAttributeChanged(name, this);

Another way to handle this is something like:

if (AXObjectCache* cache = document()->existingAXObjectCache()) ... where existingAXObjectCache() does not create. I think that would be cleaner.
Comment 9 chris fleizach 2013-03-18 15:12:58 PDT
Created attachment 193670 [details]
patch
Comment 10 chris fleizach 2013-03-18 15:13:24 PDT
(In reply to comment #9)
> Created an attachment (id=193670) [details]
> patch

This patch makes provides a way to get the existing AX object cache, instead of always creating a new one.
 12        It moves the accessibilityEnabled() checks into the axObjectCache retrieval for easier readability.
 13        It adds a number of ASSERTS so that we can make sure only the right document is used in cache manipulation (ie. the top document).
Comment 11 WebKit Review Bot 2013-03-18 15:15:45 PDT
Attachment 193670 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/ContainerNode.cpp', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/editing/AppendNodeCommand.cpp', u'Source/WebCore/editing/DeleteFromTextNodeCommand.cpp', u'Source/WebCore/editing/Editor.cpp', u'Source/WebCore/editing/InsertIntoTextNodeCommand.cpp', u'Source/WebCore/editing/InsertNodeBeforeCommand.cpp', u'Source/WebCore/editing/mac/FrameSelectionMac.mm', u'Source/WebCore/html/HTMLInputElement.cpp', u'Source/WebCore/html/HTMLSelectElement.cpp', u'Source/WebCore/html/HTMLTextFormControlElement.cpp', u'Source/WebCore/html/InputType.cpp', u'Source/WebCore/html/RangeInputType.cpp', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/platform/ScrollView.cpp', u'Source/WebCore/platform/Scrollbar.cpp', u'Source/WebCore/platform/Scrollbar.h', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderListBox.cpp', u'Source/WebCore/rendering/RenderMenuList.cpp', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObjectChildList.cpp', u'Source/WebCore/rendering/RenderText.cpp', u'Source/WebCore/rendering/RenderWidget.cpp']" exit_code: 1
Source/WebCore/page/FrameView.cpp:2508:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/html/InputType.cpp:1017:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 chris fleizach 2013-03-18 15:30:00 PDT
Created attachment 193672 [details]
patch
Comment 13 Simon Fraser (smfr) 2013-03-18 15:54:23 PDT
Comment on attachment 193672 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193672&action=review

> Source/WebCore/dom/ContainerNode.cpp:144
> +    if (documentInternal()) {
> +        if (AXObjectCache* cache = documentInternal()->existingAXObjectCache())
> +            cache->remove(this);
> +    }

The destructor seems very late to do this. Why not ContainerNode::detach()?

I assume that AXObjectCache doesn't hold refs to Node, otherwise there would be a ref cycle.

> Source/WebCore/dom/Document.cpp:3488
>  #if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) || PLATFORM(CHROMIUM)

Why just these?
Comment 14 WebKit Review Bot 2013-03-18 16:20:45 PDT
Comment on attachment 193672 [details]
patch

Attachment 193672 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17136532

New failing tests:
accessibility/disabled-controls-not-focusable.html
accessibility/aria-hidden-update.html
accessibility/canvas-fallback-content-2.html
accessibility/canvas-accessibilitynodeobject.html
accessibility/aria-readonly.html
accessibility/canvas-description-and-role.html
accessibility/canvas-fallback-content.html
accessibility/aria-labelledby-on-input.html
accessibility/color-well.html
accessibility/crash-determining-aria-role-when-label-present.html
accessibility/accessibility-node-reparent.html
accessibility/button-title-uses-inner-img-alt.html
accessibility/contenteditable-hidden-div.html
accessibility/accessibility-object-detached.html
accessibility/aria-labelledby-overrides-label.html
accessibility/aria-link-supports-press.html
accessibility/aria-labelledby-stay-within.html
accessibility/aria-toggle-button-with-title.html
accessibility/aria-slider-value.html
accessibility/aria-hidden-updates-alldescendants.html
accessibility/aria-scrollbar-role.html
accessibility/aria-labelledby-overrides-aria-label.html
accessibility/aria-describedby-on-input.html
accessibility/aria-roles.html
accessibility/accessibility-node-memory-management.html
accessibility/aria-fallback-roles.html
accessibility/dimensions-include-descendants.html
accessibility/button-press-action.html
accessibility/aria-help.html
accessibility/aria-text-role.html
Comment 15 Adam Barth 2013-03-18 17:29:45 PDT
Comment on attachment 193672 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193672&action=review

> Source/WebCore/dom/Document.cpp:2215
> +    if (!UNLIKELY(AXObjectCache::accessibilityEnabled()))

Does this UNLIKELY affect any performance measurements?  If not, we should remove it.

> Source/WebCore/dom/Document.cpp:2226
> +    if (!UNLIKELY(AXObjectCache::accessibilityEnabled()))

ditto

> Source/WebCore/dom/Document.cpp:2237
> +    ASSERT(topDocument->frame());
> +    if (!topDocument->frame())

There's no reason to both ASSERT a condition and then handle its negation.  Either the condition can occur or it can't.  If it can, the ASSERT shouldn't be there.  If it cannot, then the if-statement shouldn't be there.
Comment 16 chris fleizach 2013-03-18 17:35:03 PDT
(In reply to comment #13)
> (From update of attachment 193672 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193672&action=review
> 
> > Source/WebCore/dom/ContainerNode.cpp:144
> > +    if (documentInternal()) {
> > +        if (AXObjectCache* cache = documentInternal()->existingAXObjectCache())
> > +            cache->remove(this);
> > +    }
> 
> The destructor seems very late to do this. Why not ContainerNode::detach()?
> 

From the history I see
https://bugs.webkit.org/show_bug.cgi?id=98073

    "Calls axObjectCache()->remove(this) in ~ContainerNode so that the AX tree
    doesn't try to access the container node while walking up the parent chain
    from one of the container node's children."

> I assume that AXObjectCache doesn't hold refs to Node, otherwise there would be a ref cycle.
> 
> > Source/WebCore/dom/Document.cpp:3488
> >  #if PLATFORM(MAC) || PLATFORM(WIN) || PLATFORM(GTK) || PLATFORM(CHROMIUM)
> 

I don't know. 

I'm inclined to remove it. These are all the platforms that support accessibility, but it's not different than any other ax object cache call.

> Why just these?
Comment 17 chris fleizach 2013-03-18 17:46:25 PDT
Created attachment 193705 [details]
patch

patch addresses Simon and Adam's latest comments
Speculative fix for chromium
Comment 18 WebKit Review Bot 2013-03-18 18:36:25 PDT
Comment on attachment 193705 [details]
patch

Attachment 193705 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17235094
Comment 19 WebKit Review Bot 2013-03-18 18:43:11 PDT
Comment on attachment 193705 [details]
patch

Attachment 193705 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17201751
Comment 20 Peter Beverloo (cr-android ews) 2013-03-18 19:00:21 PDT
Comment on attachment 193705 [details]
patch

Attachment 193705 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17221365
Comment 21 Peter Beverloo (cr-android ews) 2013-03-18 19:52:25 PDT
Comment on attachment 193705 [details]
patch

Attachment 193705 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17185425
Comment 22 chris fleizach 2013-03-18 22:40:02 PDT
Created attachment 193734 [details]
patch
Comment 23 WebKit Review Bot 2013-03-18 23:37:16 PDT
Comment on attachment 193734 [details]
patch

Attachment 193734 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17160623
Comment 24 chris fleizach 2013-03-18 23:40:09 PDT
Created attachment 193744 [details]
patch
Comment 25 WebKit Review Bot 2013-03-19 00:27:59 PDT
Comment on attachment 193744 [details]
patch

Attachment 193744 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17133368

New failing tests:
accessibility/disabled-controls-not-focusable.html
accessibility/aria-hidden-update.html
accessibility/canvas-fallback-content-2.html
accessibility/canvas-accessibilitynodeobject.html
accessibility/aria-readonly.html
accessibility/canvas-description-and-role.html
accessibility/canvas-fallback-content.html
accessibility/aria-labelledby-on-input.html
accessibility/color-well.html
accessibility/crash-determining-aria-role-when-label-present.html
accessibility/accessibility-node-reparent.html
accessibility/button-title-uses-inner-img-alt.html
accessibility/contenteditable-hidden-div.html
accessibility/accessibility-object-detached.html
accessibility/aria-labelledby-overrides-label.html
accessibility/aria-link-supports-press.html
accessibility/aria-labelledby-stay-within.html
accessibility/aria-toggle-button-with-title.html
accessibility/aria-slider-value.html
accessibility/aria-hidden-updates-alldescendants.html
accessibility/aria-scrollbar-role.html
accessibility/aria-labelledby-overrides-aria-label.html
accessibility/aria-describedby-on-input.html
accessibility/aria-roles.html
accessibility/accessibility-node-memory-management.html
accessibility/aria-fallback-roles.html
accessibility/dimensions-include-descendants.html
accessibility/button-press-action.html
accessibility/aria-help.html
accessibility/aria-text-role.html
Comment 26 chris fleizach 2013-03-19 00:35:06 PDT
Dom, can you take a look at this patch? Does anything make it look like it would make chrome fail so badly with this patch?
Comment 27 Build Bot 2013-03-19 03:26:30 PDT
Comment on attachment 193744 [details]
patch

Attachment 193744 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17187559

New failing tests:
fast/css/sticky/sticky-both-sides.html
fast/css/sticky/inline-sticky.html
Comment 28 Dominic Mazzoni 2013-03-19 16:32:33 PDT
I haven't figured out exactly what's going on, but for most tests, the first accessibility object tested fails, and subsequent ones succeed. I think focusedElement returns the accessible web area the first time, then from then on it succeeds.

Some sort of initialization problem?

aria-readonly.html:
FAIL ariaTextBoxIsWritable should be true. Was false.
PASS ariaReadOnlyAriaTextBoxIsWritable is false
PASS ariaReadOnlyTextFieldIsWritable is false
PASS ariaNonReadOnlyTextFieldIsWritable is true
PASS htmlReadOnlyTextFieldIsWritable is false
PASS textFieldIsWritable is true
PASS htmlReadOnlyTextAreaIsWritable is false
PASS textAreaIsWritable is true
PASS successfullyParsed is true

aria-roles.html:
This test FAILS in DumpRenderTree. The ARIA role is AXRole: AXWebArea, but the real role is AXRole: AXCheckBox
Hello This test PASSES in DumpRenderTree. The role is AXRole: AXButton
This test PASSES in DumpRenderTree. The role is AXRole: AXHeading
Hello This test PASSES in DumpRenderTree. The role is AXRole: AXLink
This test PASSES in DumpRenderTree. The role is AXRole: AXRadioButton
 This test PASSES in DumpRenderTree. The role is AXRole: AXTextField
 This test PASSES in DumpRenderTree. The role is AXRole: AXImage
This test PASSES in DumpRenderTree. The role is AXRole: AXList
Comment 29 chris fleizach 2013-03-19 16:37:07 PDT
(In reply to comment #28)
> I haven't figured out exactly what's going on, but for most tests, the first accessibility object tested fails, and subsequent ones succeed. I think focusedElement returns the accessible web area the first time, then from then on it succeeds.
> 
> Some sort of initialization problem?
> 
> aria-readonly.html:
> FAIL ariaTextBoxIsWritable should be true. Was false.
> PASS ariaReadOnlyAriaTextBoxIsWritable is false
> PASS ariaReadOnlyTextFieldIsWritable is false
> PASS ariaNonReadOnlyTextFieldIsWritable is true
> PASS htmlReadOnlyTextFieldIsWritable is false
> PASS textFieldIsWritable is true
> PASS htmlReadOnlyTextAreaIsWritable is false
> PASS textAreaIsWritable is true
> PASS successfullyParsed is true
> 
> aria-roles.html:
> This test FAILS in DumpRenderTree. The ARIA role is AXRole: AXWebArea, but the real role is AXRole: AXCheckBox
> Hello This test PASSES in DumpRenderTree. The role is AXRole: AXButton
> This test PASSES in DumpRenderTree. The role is AXRole: AXHeading
> Hello This test PASSES in DumpRenderTree. The role is AXRole: AXLink
> This test PASSES in DumpRenderTree. The role is AXRole: AXRadioButton
>  This test PASSES in DumpRenderTree. The role is AXRole: AXTextField
>  This test PASSES in DumpRenderTree. The role is AXRole: AXImage
> This test PASSES in DumpRenderTree. The role is AXRole: AXList

Thanks. It looks like there must be one case where we are actually relying on axObjectCache() to create the AXObjectCache for us instead of just retrieving the existing one.
Comment 30 chris fleizach 2013-03-21 08:45:34 PDT
Created attachment 194271 [details]
patch

This patch will create an AXObjectCache on focus change if necessary. Looking at the failing tests in chromium indicates that might be the reason why this is failing
Comment 31 WebKit Review Bot 2013-03-21 08:48:40 PDT
Attachment 194271 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/ContainerNode.cpp', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/dom/Element.cpp', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/editing/AppendNodeCommand.cpp', u'Source/WebCore/editing/DeleteFromTextNodeCommand.cpp', u'Source/WebCore/editing/Editor.cpp', u'Source/WebCore/editing/InsertIntoTextNodeCommand.cpp', u'Source/WebCore/editing/InsertNodeBeforeCommand.cpp', u'Source/WebCore/editing/mac/FrameSelectionMac.mm', u'Source/WebCore/html/HTMLInputElement.cpp', u'Source/WebCore/html/HTMLSelectElement.cpp', u'Source/WebCore/html/HTMLTextFormControlElement.cpp', u'Source/WebCore/html/InputType.cpp', u'Source/WebCore/html/RangeInputType.cpp', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/page/Frame.cpp', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/platform/ScrollView.cpp', u'Source/WebCore/platform/Scrollbar.cpp', u'Source/WebCore/platform/Scrollbar.h', u'Source/WebCore/rendering/RenderBlock.cpp', u'Source/WebCore/rendering/RenderListBox.cpp', u'Source/WebCore/rendering/RenderMenuList.cpp', u'Source/WebCore/rendering/RenderObject.cpp', u'Source/WebCore/rendering/RenderObjectChildList.cpp', u'Source/WebCore/rendering/RenderText.cpp', u'Source/WebCore/rendering/RenderWidget.cpp']" exit_code: 1
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 WebKit Review Bot 2013-03-21 10:18:43 PDT
Comment on attachment 194271 [details]
patch

Attachment 194271 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17206705

New failing tests:
accessibility/disabled-controls-not-focusable.html
accessibility/canvas-fallback-content-2.html
platform/chromium/accessibility/add-to-menu-list-crashes.html
accessibility/canvas-accessibilitynodeobject.html
editing/selection/selection-invalid-offset.html
accessibility/canvas-fallback-content.html
platform/chromium/accessibility/readonly.html
accessibility/menu-list-sends-change-notification.html
Comment 33 WebKit Review Bot 2013-03-21 10:18:47 PDT
Created attachment 194287 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 34 chris fleizach 2013-03-21 12:33:21 PDT
Created attachment 194317 [details]
patch
Comment 35 WebKit Review Bot 2013-03-21 14:10:19 PDT
Comment on attachment 194317 [details]
patch

Attachment 194317 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17147640

New failing tests:
editing/selection/selection-invalid-offset.html
Comment 36 WebKit Review Bot 2013-03-21 14:10:23 PDT
Created attachment 194335 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 37 chris fleizach 2013-03-21 15:38:01 PDT
Created attachment 194366 [details]
patch
Comment 38 WebKit Review Bot 2013-03-21 17:16:49 PDT
Comment on attachment 194366 [details]
patch

Attachment 194366 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17216561

New failing tests:
editing/selection/selection-invalid-offset.html
Comment 39 WebKit Review Bot 2013-03-21 17:16:55 PDT
Created attachment 194392 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 40 chris fleizach 2013-03-21 17:31:29 PDT
Created attachment 194397 [details]
patch
Comment 41 WebKit Review Bot 2013-03-21 18:51:51 PDT
Comment on attachment 194397 [details]
patch

Attachment 194397 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17215997

New failing tests:
editing/selection/selection-invalid-offset.html
Comment 42 WebKit Review Bot 2013-03-21 18:51:57 PDT
Created attachment 194416 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 43 WebKit Review Bot 2013-03-21 20:14:37 PDT
Comment on attachment 194397 [details]
patch

Attachment 194397 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17144564

New failing tests:
editing/selection/selection-invalid-offset.html
Comment 44 WebKit Review Bot 2013-03-21 20:14:43 PDT
Created attachment 194428 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 45 chris fleizach 2013-03-22 17:58:52 PDT
Created attachment 194672 [details]
patch

Thanks Dom for the chromium help!
Comment 46 chris fleizach 2013-03-24 00:40:13 PDT
http://trac.webkit.org/changeset/146726