AXObjectCache gets recreated during document tear-down

[Simon Fraser (smfr)]

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

[chris fleizach]

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

[Simon Fraser (smfr)]

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

[chris fleizach]

Created attachment 193595 [details]
patch

[WebKit Review Bot]

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.

[Adam Barth]

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.

[chris fleizach]

Created attachment 193597 [details]
patch

[Adam Barth]

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.

[Simon Fraser (smfr)]

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.

[chris fleizach]

Created attachment 193670 [details]
patch

[chris fleizach]

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

[WebKit Review Bot]

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.

[chris fleizach]

Created attachment 193672 [details]
patch

[Simon Fraser (smfr)]

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?

[WebKit Review Bot]

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

[Adam Barth]

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.

[chris fleizach]

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

[chris fleizach]

Created attachment 193705 [details]
patch

patch addresses Simon and Adam's latest comments
Speculative fix for chromium

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Peter Beverloo (cr-android ews)]

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

[Peter Beverloo (cr-android ews)]

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

[chris fleizach]

Created attachment 193734 [details]
patch

[WebKit Review Bot]

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

[chris fleizach]

Created attachment 193744 [details]
patch

[WebKit Review Bot]

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

[chris fleizach]

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?

[Build Bot]

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

[Dominic Mazzoni]

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

[chris fleizach]

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

[chris fleizach]

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

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[chris fleizach]

Created attachment 194317 [details]
patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[chris fleizach]

Created attachment 194366 [details]
patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[chris fleizach]

Created attachment 194397 [details]
patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[chris fleizach]

Created attachment 194672 [details]
patch

Thanks Dom for the chromium help!

[chris fleizach]

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