Bug 233014 - Fix for crash in LayoutTests in isolated tree mode.
Summary: Fix for crash in LayoutTests in isolated tree mode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-11 14:12 PST by Andres Gonzalez
Modified: 2021-11-15 16:50 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.92 KB, patch)
2021-11-11 14:18 PST, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2021-11-11 14:12:38 PST
Fix for crash in LayoutTests in isolated tree mode.
Comment 1 Radar WebKit Bug Importer 2021-11-11 14:12:52 PST
<rdar://problem/85315168>
Comment 2 Andres Gonzalez 2021-11-11 14:18:05 PST
Created attachment 444000 [details]
Patch
Comment 3 EWS 2021-11-11 15:38:41 PST
Committed r285677 (244160@main): <https://commits.webkit.org/244160@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444000 [details].
Comment 4 Darin Adler 2021-11-11 16:18:35 PST
Comment on attachment 444000 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:582
> -        NSString *value = descriptionOfValue(attributeValue(attribute).get());
> -        [values appendFormat:@"%@: %@\n", attribute, value];
> +        RetainPtr<NSString> value = descriptionOfValue(attributeValue(attribute).get());
> +        [values appendFormat:@"%@: %@\n", attribute, value.get()];

I don’t understand this. Why do we need to retain the string?
Comment 5 Andres Gonzalez 2021-11-15 16:50:52 PST
(In reply to Darin Adler from comment #4)
> Comment on attachment 444000 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444000&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:582
> > -        NSString *value = descriptionOfValue(attributeValue(attribute).get());
> > -        [values appendFormat:@"%@: %@\n", attribute, value];
> > +        RetainPtr<NSString> value = descriptionOfValue(attributeValue(attribute).get());
> > +        [values appendFormat:@"%@: %@\n", attribute, value.get()];
> 
> I don’t understand this. Why do we need to retain the string?

Darin, I don't have a good explanation for this, but this doesn't crash any longer. The other way I found to avoid this crash is as follows:

-    NSString *description = descriptionOfValue(attributeValue(NSAccessibilityValueAttribute).get()); 

// the above crashes when you dereference description in any way, like description.length.

+    auto value = attributeValue(NSAccessibilityValueAttribute);
+    NSString *description = descriptionOfValue(value.get()); 

// doesn't crash if you description.length.

attributeValue is doing some threading trickery because it is dispatching to a secondary, mocked thread and waiting, and then spinning the main thread run loop until the dispatched block is executed on the secondary thread. My hypothesis is that this is allowing the garbage collector to kick in and release the return value of attributeValue, unless is assigned to an lvalue.

The stack trace of the crash is not inside descriptionOfValue though, but when dereferencing the description string which makes no much sense to me...

would greatly appreciate it if you had any suggestion. Thanks!