Bug 149217 - AX: DRT crash at WebCore: WebCore::AccessibilityObject::nextSiblingUnignored(int) const
Summary: AX: DRT crash at WebCore: WebCore::AccessibilityObject::nextSiblingUnignored(...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: Safari 9
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-09-16 09:28 PDT by chris fleizach
Modified: 2015-09-20 14:18 PDT (History)
10 users (show)

See Also:


Attachments
patch (6.74 KB, patch)
2015-09-16 14:07 PDT, chris fleizach
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2015-09-16 09:28:30 PDT
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>
Comment 1 chris fleizach 2015-09-16 14:07:11 PDT
Created attachment 261325 [details]
patch
Comment 2 Alexey Proskuryakov 2015-09-16 14:23:50 PDT
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.
Comment 3 chris fleizach 2015-09-16 14:26:33 PDT
(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
Comment 4 Alexey Proskuryakov 2015-09-16 14:50:34 PDT
> 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.
Comment 5 chris fleizach 2015-09-16 15:13:41 PDT
(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
Comment 6 Alexey Proskuryakov 2015-09-16 15:25:42 PDT
> 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.
Comment 7 chris fleizach 2015-09-16 15:30:12 PDT
(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
Comment 8 Alexey Proskuryakov 2015-09-19 23:48:57 PDT
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.
Comment 9 Alexey Proskuryakov 2015-09-20 12:56:04 PDT
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.
Comment 10 chris fleizach 2015-09-20 13:11:17 PDT
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.
Comment 11 Alexey Proskuryakov 2015-09-20 14:18:23 PDT
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.