Bug 237373 – AX: [WebAccessibilityObjectWrapperMac AXAttributeStringSetFont] crashes when given a font with a nil postscript name, font family, or display name

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

[WebAccessibilityObjectWrapperMac AXAttributeStringSetFont] crashes when given a font with a nil postscript name, font family, or display name. This can be triggered with CSS:

@font-face {
    font-family: 'Litherum';
    src: url("resources/Litherum.svg") format(svg);
}
* { font-family: 'Litherum'; }

[Radar WebKit Bug Importer]

<rdar://problem/89690380>

[Tyler Wilcock]

Created attachment 453617 [details]
Patch

[Tyler Wilcock]

Created attachment 453621 [details]
Patch

[chris fleizach]

Comment on attachment 453621 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:784
> +        NSString *fontName = (__bridge NSString *)adoptCF(CTFontCopyPostScriptName(font)).get();

can we take this opportunity to centralize both of these methods which appear to be mostly duplicated

[Tyler Wilcock]

Created attachment 453664 [details]
Patch

[chris fleizach]

Comment on attachment 453664 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:602
> +    RetainPtr<CFStringRef> fullName = adoptCF(CTFontCopyFullName(font));

it looks like on the Mac side we're adding a whole dictionary and on iOS side we're doing addAttribute:value for each item

Can we pick one way (like make a dictionary with the right platform keys)
and then set it one time?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:806
> +    [WebAccessibilityObjectWrapperBase applyFontToAttributedString:attrString font:style.fontCascade().primaryFont().getCTFont() range:range];

can we make this a C function instead of a class function?

[Tyler Wilcock]

Created attachment 453680 [details]
Patch

[chris fleizach]

Comment on attachment 453680 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h:112
> ++ (void)applyFontToAttributedString:(NSMutableAttributedString*)attributedString font:(CTFontRef)font range:(NSRange)range;

NSMutableAttributedString *

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:585
> +    return range.location < [attributedString length] && NSMaxRange(range) <= [attributedString length];

the NSMaxRange check should be sufficient. I don't think you need the range.location check

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:593
> +    if (!font) {

we can wrap this whole block in MAC

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:600
> +    NSMutableDictionary *fontAttributes = [[NSMutableDictionary alloc] init];

we should RetainPtr this or release it. pretty sure ARC is not on

[Tyler Wilcock]

Created attachment 453685 [details]
Patch

[Tyler Wilcock]

> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:593
> > +    if (!font) {
> 
> we can wrap this whole block in MAC
Can we? We do still need to return early if the font is null on iOS.

Fixed everything else, and made them C functions as you requested (I think it's nicer too).

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #9)
> Created attachment 453685 [details]
> Patch

--- a/Source/WebCore/ChangeLog
+++ a/Source/WebCore/ChangeLog

+        Test: accessibility/svg-font-face-should-not-crash.html

Can we rename this test to just svg-font-face.html. Nothing should crash, so it is extra typing and reading with no valuable information. It is like adding accessibility to the name of accessibility tests under the accessibility dir.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #9)
> Created attachment 453685 [details]
> Patch

--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h
+++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h

+#import "FontPlatformData.h"

Can we forward declare what we need from this header and #import in the .mm instead. This helps with compile time.

[Andres Gonzalez]

(In reply to Andres Gonzalez from comment #12)
> (In reply to Tyler Wilcock from comment #9)
> > Created attachment 453685 [details]
> > Patch
> 
> --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h
> +++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h
> 
> +#import "FontPlatformData.h"
> 
> Can we forward declare what we need from this header and #import in the .mm
> instead. This helps with compile time.

+void AXAttributeStringSetFont(NSMutableAttributedString*, CTFontRef, NSRange);

Shouldn't this be AXAttributedStringSetFont instead of AXAttributeStringSetFont?

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #9)
> Created attachment 453685 [details]
> Patch

--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm
+++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm

+bool AXAttributedStringRangeIsValid(NSAttributedString *attributedString, NSRange range)
+{
+    return NSMaxRange(range) <= [attributedString length];
+}

Do we need a function to make this check? I believe it is more readable to have the check inline where it is needed. Otherwise you would be wondering what AXAttributedStringRangeIsValid is doing?

+void AXAttributeStringSetFont(NSMutableAttributedString* attributedString, CTFontRef font, NSRange range)

Shouldn't the name be AXAttributedStringSetFont?

NSMutableAttributedString* attributedString has the * in the wrong side.

Shouldn't we pass NSRange by reference?

+    RetainPtr<NSMutableDictionary> fontAttributes = adoptNS([[NSMutableDictionary alloc] init]);
+    RetainPtr<CFStringRef> familyName = adoptCF(CTFontCopyFamilyName(font));

Do we need to retain all these local variables?

     NSNumber* size = [NSNumber numberWithFloat:CTFontGetSize(font)];

* in the wrong side.

-        [attrString addAttribute:UIAccessibilityTokenFontName value:(NSString*)fullName.get() range:range];
+        [fontAttributes setValue:(__bridge NSString*)fullName.get() forKey:UIAccessibilityTokenFontName];

Use bridge_cast instead of (__bridge NSString*).

+    CTFontSymbolicTraits traits = CTFontGetSymbolicTraits(font);

Can we use auto here?

+        [fontAttributes setValue:(__bridge NSString*)familyName.get() forKey:NSAccessibilityFontFamilyKey];

bridge_cast

+        [fontAttributes setValue:(__bridge NSString*)postScriptName.get() forKey:NSAccessibilityFontNameKey];

bridge_cast

+        [fontAttributes setValue:(__bridge NSString*)displayName.get() forKey:NSAccessibilityVisibleNameKey];

bridge_cast

[Tyler Wilcock]

Created attachment 453760 [details]
Patch

[Tyler Wilcock]

Created attachment 453788 [details]
Patch

[chris fleizach]

Comment on attachment 453788 [details]
Patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h:61
> +bool AXAttributedStringRangeIsValid(NSAttributedString*, NSRange);

* in wrong place

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #16)
> Created attachment 453788 [details]
> Patch

--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h
+++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.h

+bool AXAttributedStringRangeIsValid(NSAttributedString*, NSRange);

if we are keeping this function, if should be:

bool AXAttributedStringRangeIsValid(NSAttributedString *, const NSRange&);

or we can get rid of it altogether.

+void AXAttributedStringSetFont(NSMutableAttributedString*, CTFontRef, const NSRange&);

NSMutableAttributedString* -> NSMutableAttributedString *

--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm
+++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm

+bool AXAttributedStringRangeIsValid(NSAttributedString *attributedString, NSRange range)

Same as above, remove or pass NSRange by reference.

[Tyler Wilcock]

Created attachment 453841 [details]
Patch

[Tyler Wilcock]

Created attachment 453842 [details]
Patch

[EWS]

Committed r290858 (248089@main): <https://commits.webkit.org/248089@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453842 [details].