Bug 149217 – AX: DRT crash at WebCore: WebCore::AccessibilityObject::nextSiblingUnignored(int) const

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

WTF::PtrHash<WebCore::RenderObject*>, WTF::HashMap<WebCore::RenderObject*, unsigned int, WTF::PtrHash<WebCore::RenderObject*>, WTF::HashTraits<WebCore::RenderObject*>, WTF::HashTraits<unsigned int> >::KeyValuePairTraits, WTF::HashTraits<WebCore::RenderObject*> >::lookup<WTF::IdentityHashTranslator<WTF::PtrHash<WebCore::RenderObject*> >, WebCore::RenderObject*>(WebCore::RenderObject* const&) <==
        5 WebCore: WebCore::AXObjectCache::get(WebCore::RenderObject*)
          5 WebCore: WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*)
            5 WebCore: WebCore::AccessibilityObject::nextSiblingUnignored(int) const
              5 WebCore: WebCore::AXObjectCache::postTextStateChangeNotification(WebCore::Position const&, WebCore::AXTextStateChangeIntent const&, WebCore::VisibleSelection const&)
                5 WebCore: WebCore::FrameSelection::notifyAccessibilityForSelectionChange(WebCore::AXTextStateChangeIntent const&)
                  5 WebCore: WebCore::FrameSelection::updateAndRevealSelection(WebCore::AXTextStateChangeIntent const&)
                    5 WebCore: WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, unsigned int, WebCore::AXTextStateChangeIntent, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)
                      5 WebCore: WebCore::FrameSelection::moveWithoutValidationTo(WebCore::Position const&, WebCore::Position const&, bool, bool, WebCore::AXTextStateChangeIntent const&)
                        5 WebCore: WebCore::HTMLTextFormControlElement::setSelectionRange(int, int, WebCore::TextFieldSelectionDirection, WebCore::AXTextStateChangeIntent const&)
                          5 WebCore: WebCore::HTMLTextAreaElement::setValueCommon(WTF::String const&)
                            5 WebCore: WebCore::HTMLTextAreaElement::setValue(WTF::String const&)
                              5 WebCore: WebCore::setJSHTMLTextAreaElementValue(JSC::ExecState*, JSC::JSObject*, long long, long long)

<rdar://problem/22488928>

[chris fleizach]

Created attachment 261325 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 261325 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +        When DRT runs accessibilityEnabled is turned on and off regularly. As a result, some objects like axObjectCache()
> +        are null unexpectedly. Does not happen to users because once enabled, there's no way to turn accessibility off.

Can this be fixed in DRT? Adding so much production code just to work around DRT misbehavior seems unfortunate and fragile.

[chris fleizach]

(In reply to comment #2)
> Comment on attachment 261325 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=261325&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        When DRT runs accessibilityEnabled is turned on and off regularly. As a result, some objects like axObjectCache()
> > +        are null unexpectedly. Does not happen to users because once enabled, there's no way to turn accessibility off.
> 
> Can this be fixed in DRT? Adding so much production code just to work around
> DRT misbehavior seems unfortunate and fragile.

i don't think so. maybe if we made sure every test was cleaned up completely, the document was torn down and then the next test had a whole clean world it could fix it but that situation is what triggers this bug

Also, i think this production code changes are a good thing. In theory they could happen to users so better to be safe if layout tests are able to catch these edge case-y things

[Alexey Proskuryakov]

> i don't think so. maybe if we made sure every test was cleaned up completely, the document was torn down and then the next test had a whole clean world it could fix it but that situation is what triggers this bug

We already do that - we even load about:blank between tests to make sure that the state is reset very well. Could it be just a matter of disabling accessibility a little later than we do it now?

Having in-flight unhandled messages likely also makes accessibility tests flaky in other ways.

> Also, i think this production code changes are a good thing. In theory they could happen to users so better to be safe if layout tests are able to catch these edge case-y things

Could you please elaborate on how this can happen to users? If so, then we certainly need to fix this in code - but the ChangeLog even explains why this can't happen to users.

[chris fleizach]

(In reply to comment #4)
> > i don't think so. maybe if we made sure every test was cleaned up completely, the document was torn down and then the next test had a whole clean world it could fix it but that situation is what triggers this bug
> 
> We already do that - we even load about:blank between tests to make sure
> that the state is reset very well. Could it be just a matter of disabling
> accessibility a little later than we do it now?

I think these back traces show that this is not the case. in this crash trace, accessibility is turned off even though the test has not completed.

> 
> Having in-flight unhandled messages likely also makes accessibility tests
> flaky in other ways.
> 
> > Also, i think this production code changes are a good thing. In theory they could happen to users so better to be safe if layout tests are able to catch these edge case-y things
> 
> Could you please elaborate on how this can happen to users? If so, then we
> certainly need to fix this in code - but the ChangeLog even explains why
> this can't happen to users.

I don't know how it might happen but there's lots of variations on usage I am not aware of. Adding checks for objects that can return nil however seems like a good thing through and through

[Alexey Proskuryakov]

> I think these back traces show that this is not the case. in this crash trace, accessibility is turned off even though the test has not completed.

The crash happens while loading about:blank. I think that it will be fixed if we don't disable accessibility before loading about:blank.

[chris fleizach]

(In reply to comment #6)
> > I think these back traces show that this is not the case. in this crash trace, accessibility is turned off even though the test has not completed.
> 
> The crash happens while loading about:blank. I think that it will be fixed
> if we don't disable accessibility before loading about:blank.

probably

[Alexey Proskuryakov]

Comment on attachment 261325 [details]
patch

I'm looking into fixing DumpRenderTree, r- for now.

There is another interesting thing happening - behavior of accessibility/mac/removing-textarea-after-edit-crash.html depends on whether accessibility/mac/loaded-notification.html ran before it. That's not good too.

[Alexey Proskuryakov]

Landed just the test fix in r190033. Between that and a DRT fix in bug 149384, the test doesn't crash any more.

The problem with unexpectedly changing WebCore global state during a test still remains, I'm going to keep working on it. It's not a simple one-line change - there is some state that we do want to reset before loading about:blank, notably content blocking.

[chris fleizach]

Thanks for digging deeper on this one

Looks like it will prevent future issues as well


(In reply to comment #9)
> Landed just the test fix in r190033. Between that and a DRT fix in bug
> 149384, the test doesn't crash any more.
> 
> The problem with unexpectedly changing WebCore global state during a test
> still remains, I'm going to keep working on it. It's not a simple one-line
> change - there is some state that we do want to reset before loading
> about:blank, notably content blocking.

[Alexey Proskuryakov]

Another related fix in bug 149391. Naively moving the reset function introduces a problem with user scripts, and turned out that we already had this problem in WebKit2.