Bug 135074 – Clicking on links while accessibility is enabled sometimes crashes

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Myles C. Maxfield]

Clicking on links while accessibility is enabled does not render as expected

[Myles C. Maxfield]

Created attachment 235154 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/16826229>

[Build Bot]

Comment on attachment 235154 [details]
Patch

Attachment 235154 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4863039476072448

New failing tests:
accessibility/plugin.html
platform/mac/accessibility/aria-columnrowheaders.html
platform/mac/accessibility/document-attributes.html
accessibility/image-link.html
platform/mac/accessibility/document-links.html
accessibility/table-attributes.html
accessibility/table-sections.html
platform/mac/accessibility/internal-link-anchors.html
accessibility/table-one-cell.html
accessibility/table-detection.html
platform/mac/accessibility/bounds-for-range.html
accessibility/table-cells.html
accessibility/image-map2.html
accessibility/table-cell-spans.html
accessibility/table-with-rules.html
accessibility/transformed-element.html
accessibility/internal-link-anchors2.html
accessibility/lists.html

[Build Bot]

Created attachment 235159 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 235154 [details]
Patch

Attachment 235154 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6298681250676736

New failing tests:
accessibility/plugin.html
platform/mac/accessibility/aria-columnrowheaders.html
platform/mac/accessibility/document-attributes.html
accessibility/image-link.html
platform/mac/accessibility/document-links.html
accessibility/table-attributes.html
accessibility/table-sections.html
platform/mac/accessibility/internal-link-anchors.html
accessibility/table-one-cell.html
accessibility/table-detection.html
platform/mac/accessibility/bounds-for-range.html
accessibility/table-cells.html
accessibility/image-map2.html
accessibility/table-cell-spans.html
accessibility/table-with-rules.html
accessibility/transformed-element.html
accessibility/internal-link-anchors2.html
accessibility/lists.html

[Build Bot]

Created attachment 235163 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 235154 [details]
Patch

Attachment 235154 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4650046410719232

New failing tests:
accessibility/plugin.html
platform/mac/accessibility/aria-columnrowheaders.html
platform/mac/accessibility/document-attributes.html
accessibility/image-link.html
platform/mac/accessibility/document-links.html
accessibility/table-attributes.html
accessibility/table-sections.html
platform/mac/accessibility/internal-link-anchors.html
accessibility/table-one-cell.html
accessibility/table-detection.html
platform/mac/accessibility/bounds-for-range.html
accessibility/table-cells.html
accessibility/image-map2.html
media/track/track-long-word-container-sizing.html
accessibility/table-cell-spans.html
accessibility/table-with-rules.html
accessibility/transformed-element.html
accessibility/internal-link-anchors2.html
accessibility/lists.html

[Build Bot]

Created attachment 235165 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[chris fleizach]

Comment on attachment 235154 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:-2766
> -            return nil;

It looks like this is causing the test regressions. a bunch of tests except a nil. 

I think it might be good to have this in a different patch if possible, so that we can have a very focused patch just on updateBackingStore() issue

[Myles C. Maxfield]

Chris: How would you feel if I updated the tests? Making sure that we adhere to incorrect behavior seems bad.

[chris fleizach]

(In reply to comment #10)
> Chris: How would you feel if I updated the tests? Making sure that we adhere to incorrect behavior seems bad.

i think we need a separate bug to handle the issue from rdar://16826229. that's my only concern. thanks

[Myles C. Maxfield]

Created attachment 235255 [details]
Patch

[Darin Adler]

Comment on attachment 235255 [details]
Patch

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

> Source/WebCore/ChangeLog:26
> +        (WebCore::AccessibilityObject::updateBackingStore): Problem 3 above.

There is no problem 3 above.

> Source/WebCore/accessibility/AccessibilityObject.cpp:1440
> +    if (!hasOneRef())
> +        updateChildrenIfNecessary();

Why this hasOneRef() optimization? Is it for correctness or efficiency? Maybe it’s explained in the mysterious “problem 3” discussion?

[Darin Adler]

Comment on attachment 235255 [details]
Patch

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

It’s gratuitous to include all those nullptr changes in this patch. Should just land them separately so this patch is small and to the point.

>> Source/WebCore/ChangeLog:26
>> +        (WebCore::AccessibilityObject::updateBackingStore): Problem 3 above.
> 
> There is no problem 3 above.

Oh, I see, it’s problem 2 above.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1440
>> +        updateChildrenIfNecessary();
> 
> Why this hasOneRef() optimization? Is it for correctness or efficiency? Maybe it’s explained in the mysterious “problem 3” discussion?

Now that I understand “problem 2”, I believe firmly that the hasOneRef optimization is not helpful. I suggest you omit it for simplicity, even though it’s kind of neat to save the work. I don’t think we need to optimize this kind of unusual edge case, just prevent it from crashing.

[Myles C. Maxfield]

Comment on attachment 235255 [details]
Patch

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

>> Source/WebCore/ChangeLog:26
>> +        (WebCore::AccessibilityObject::updateBackingStore): Problem 3 above.
> 
> There is no problem 3 above.

Whoops.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1440
>> +        updateChildrenIfNecessary();
> 
> Why this hasOneRef() optimization? Is it for correctness or efficiency? Maybe it’s explained in the mysterious “problem 3” discussion?

Efficiency. No reason to update children if we are going to die momentarily.

[Myles C. Maxfield]

Comment on attachment 235255 [details]
Patch

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

>>>> Source/WebCore/accessibility/AccessibilityObject.cpp:1440
>>>> +        updateChildrenIfNecessary();
>>> 
>>> Why this hasOneRef() optimization? Is it for correctness or efficiency? Maybe it’s explained in the mysterious “problem 3” discussion?
>> 
>> Now that I understand “problem 2”, I believe firmly that the hasOneRef optimization is not helpful. I suggest you omit it for simplicity, even though it’s kind of neat to save the work. I don’t think we need to optimize this kind of unusual edge case, just prevent it from crashing.
> 
> Efficiency. No reason to update children if we are going to die momentarily.

Alright, I'll remove the if

[Myles C. Maxfield]

Created attachment 235257 [details]
Patch

[Myles C. Maxfield]

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