[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'; }
<rdar://problem/89690380>
Created attachment 453617 [details] Patch
Created attachment 453621 [details] Patch
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
Created attachment 453664 [details] Patch
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?
Created attachment 453680 [details] Patch
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
Created attachment 453685 [details] Patch
> > 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).
(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.
(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.
(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?
(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
Created attachment 453760 [details] Patch
Created attachment 453788 [details] Patch
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
(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.
Created attachment 453841 [details] Patch
Created attachment 453842 [details] Patch
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].