Bug 135074 - Clicking on links while accessibility is enabled sometimes crashes
Summary: Clicking on links while accessibility is enabled sometimes crashes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-18 15:36 PDT by Myles C. Maxfield
Modified: 2014-07-22 10:51 PDT (History)
20 users (show)

See Also:


Attachments
Patch (8.01 KB, patch)
2014-07-18 16:05 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (669.57 KB, application/zip)
2014-07-18 17:31 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (674.26 KB, application/zip)
2014-07-18 18:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (1.27 MB, application/zip)
2014-07-18 20:38 PDT, Build Bot
no flags Details
Patch (9.89 KB, patch)
2014-07-21 17:10 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (5.15 KB, patch)
2014-07-21 17:29 PDT, Myles C. Maxfield
cfleizach: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-07-18 15:36:28 PDT
Clicking on links while accessibility is enabled does not render as expected
Comment 1 Myles C. Maxfield 2014-07-18 16:05:29 PDT
Created attachment 235154 [details]
Patch
Comment 2 Myles C. Maxfield 2014-07-18 16:06:08 PDT
<rdar://problem/16826229>
Comment 3 Build Bot 2014-07-18 17:30:56 PDT
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
Comment 4 Build Bot 2014-07-18 17:31:01 PDT
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
Comment 5 Build Bot 2014-07-18 18:36:20 PDT
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
Comment 6 Build Bot 2014-07-18 18:36:25 PDT
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
Comment 7 Build Bot 2014-07-18 20:37:58 PDT
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
Comment 8 Build Bot 2014-07-18 20:38:03 PDT
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
Comment 9 chris fleizach 2014-07-18 22:25:38 PDT
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
Comment 10 Myles C. Maxfield 2014-07-21 11:27:30 PDT
Chris: How would you feel if I updated the tests? Making sure that we adhere to incorrect behavior seems bad.
Comment 11 chris fleizach 2014-07-21 11:31:51 PDT
(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
Comment 12 Myles C. Maxfield 2014-07-21 17:10:47 PDT
Created attachment 235255 [details]
Patch
Comment 13 Darin Adler 2014-07-21 17:13:26 PDT
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?
Comment 14 Darin Adler 2014-07-21 17:16:54 PDT
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.
Comment 15 Myles C. Maxfield 2014-07-21 17:22:56 PDT
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.
Comment 16 Myles C. Maxfield 2014-07-21 17:24:10 PDT
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
Comment 17 Myles C. Maxfield 2014-07-21 17:29:55 PDT
Created attachment 235257 [details]
Patch
Comment 18 Myles C. Maxfield 2014-07-22 10:51:11 PDT
http://trac.webkit.org/changeset/171347