Bug 202198 - REGRESSION (r245672): <select> dropdown with text-rendering: optimizeLegibility freezes Safari
Summary: REGRESSION (r245672): <select> dropdown with text-rendering: optimizeLegibili...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: Other
Hardware: Macintosh macOS 10.14
: P2 Critical
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-25 04:47 PDT by yann.lohier
Modified: 2019-12-02 23:29 PST (History)
20 users (show)

See Also:


Attachments
Patch (6.92 KB, patch)
2019-10-01 19:18 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (6.96 KB, patch)
2019-10-01 19:23 PDT, Myles C. Maxfield
thorton: review+
Details | Formatted Diff | Diff
Patch for committing (6.98 KB, patch)
2019-10-02 11:30 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (6.98 KB, patch)
2019-10-02 15:10 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description yann.lohier 2019-09-25 04:47:35 PDT
Hi,

Here is a test case: the dropdown has a custom appearance and inherits text-rendering: optimizeLegibility from the body.
Click it to crash the page.


<!doctype html>
<html>
	<head>
		<style>
			body
			{
				text-rendering: optimizeLegibility;
			}
			
			select
			{
				border: 1px solid red;
				height: 32px;
				width: 200px;
				-webkit-appearance: none;
				/* text-rendering: auto; uncomment and it's fine */
			}
		</style>
	</head>
	<body>
		<select>
			<option>click</option>
			<option>click</option>
		</select>
	</body>
</html>
Comment 1 Alexey Proskuryakov 2019-09-25 15:17:16 PDT
I cannot reproduce this. Could you please attach a complete crash log?
Comment 2 yann.lohier 2019-09-25 16:23:45 PDT
If that helps: this bug crashes Safari 13 (and 13.01) on both my Mac Mini (2012) and my Macbook (2014). Both run Mojave.

How it happens:
- I click the button,
- the menu doesn't show up and the Safari interface is stuck.
- After that, the beachball appears and the only thing I can do is to force quit Safari.

I've sent you the report by email (I don't know if it contains sensitive informations. I doubt it, but just in case...).
Comment 3 Alexey Proskuryakov 2019-09-26 08:54:47 PDT
Thank you!

rdar://problem/55744833
Comment 4 Alexey Proskuryakov 2019-09-29 15:22:05 PDT
*** Bug 202262 has been marked as a duplicate of this bug. ***
Comment 5 Myles C. Maxfield 2019-10-01 12:19:53 PDT
I can reproduce this.
Comment 6 Myles C. Maxfield 2019-10-01 12:50:07 PDT
Oh, it looks like [NSFont fontWithDescriptor:size:] throws an Objective-C exception:

-[NSTaggedPointerString _getValue:forType:]: unrecognized selector

We never surrounded this code with try/catch blocks, so we pop back up to the runloop, thereby confusing the UI process.
Comment 7 Myles C. Maxfield 2019-10-01 14:16:33 PDT
The font descriptor is identical on Mojave and Catalina:

NSCTFontDescriptor <0x60000260bd80> = {
    CTFontDescriptorLanguageAttribute = "";
    NSCTFontCascadeListAttribute =     (
    );
    NSCTFontOpticalSizeAttribute = auto;
    NSCTFontUIUsageAttribute = CTFontRegularUsage;
    NSFontSizeAttribute = 11;
}
Comment 8 Myles C. Maxfield 2019-10-01 15:15:48 PDT
NSCTFontOpticalSizeAttribute is the field that's tripping up NSFont.
Comment 9 Myles C. Maxfield 2019-10-01 15:19:45 PDT
Caused by r245672.
Comment 10 Myles C. Maxfield 2019-10-01 15:20:48 PDT
<rdar://problem/55744833>
Comment 11 Myles C. Maxfield 2019-10-01 19:18:54 PDT
Created attachment 379981 [details]
Patch
Comment 12 Myles C. Maxfield 2019-10-01 19:23:27 PDT
Created attachment 379982 [details]
Patch
Comment 13 Tim Horton 2019-10-01 19:58:38 PDT
Comment on attachment 379982 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1602
> +#define USE_CORETEXT_AUTO_OPTICAL_SIZING_WORKAROUND 1

Pretty sure this is a HAVE() not a USE() (which is about using features of the platform, not internal workarounds)

HAVE_CORETEXT_WITH_BROKEN_AUTO_OPTICAL_SIZING or vice versa or something like that

> Source/WebKit/UIProcess/mac/WebPopupMenuProxyMac.mm:109
> +        NSMutableDictionary *mutableDictionary = [(__bridge NSDictionary *)data.fontInfo.fontAttributeDictionary.get() mutableCopy];

Why not a RetainPtr & adoptNS?

> Source/WebKit/UIProcess/mac/WebPopupMenuProxyMac.mm:115
> +                [mutableDictionary removeObjectForKey:(NSString *)kCTFontOpticalSizeAttribute];
> +                if (NSNumber *size = [mutableDictionary objectForKey:(NSString *)kCTFontSizeAttribute])
> +                    [mutableDictionary setObject:size forKey:(NSString *)kCTFontOpticalSizeAttribute];

Some might say if you're going to include one bridge you might as well bridge them all. It's up to you since it really doesn't matter yet.
Comment 14 Myles C. Maxfield 2019-10-02 11:30:53 PDT
Created attachment 380042 [details]
Patch for committing
Comment 15 Myles C. Maxfield 2019-10-02 15:10:12 PDT
Created attachment 380063 [details]
Patch for committing
Comment 16 WebKit Commit Bot 2019-10-02 17:53:38 PDT
Comment on attachment 380063 [details]
Patch for committing

Clearing flags on attachment: 380063

Committed r250640: <https://trac.webkit.org/changeset/250640>
Comment 17 Alexey Proskuryakov 2019-10-03 10:45:37 PDT
Does this fix address the dupe (bug 202262)?
Comment 18 Myles C. Maxfield 2019-10-03 18:33:26 PDT
(In reply to Alexey Proskuryakov from comment #17)
> Does this fix address the dupe (bug 202262)?

Nope! https://bugs.webkit.org/show_bug.cgi?id=202262
Comment 19 Darin Adler 2019-10-08 09:39:39 PDT
Comment on attachment 380063 [details]
Patch for committing

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

> Source/WebKit/UIProcess/mac/WebPopupMenuProxyMac.mm:124
> +    END_BLOCK_OBJC_EXCEPTIONS

If an exception is blocked here, font may be nil. Does the code below handle that?
Comment 20 Darin Adler 2019-10-08 09:40:14 PDT
Comment on attachment 380063 [details]
Patch for committing

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

>> Source/WebKit/UIProcess/mac/WebPopupMenuProxyMac.mm:124
>> +    END_BLOCK_OBJC_EXCEPTIONS
> 
> If an exception is blocked here, font may be nil. Does the code below handle that?

I suggest rearranging the code so that font will be set to [NSFont menuFontOfSize:0] if there is an exception.
Comment 21 Darin Adler 2019-10-08 09:41:22 PDT
Comment on attachment 380063 [details]
Patch for committing

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

>>> Source/WebKit/UIProcess/mac/WebPopupMenuProxyMac.mm:124
>>> +    END_BLOCK_OBJC_EXCEPTIONS
>> 
>> If an exception is blocked here, font may be nil. Does the code below handle that?
> 
> I suggest rearranging the code so that font will be set to [NSFont menuFontOfSize:0] if there is an exception.

Like this:

    NSFont *font = nil;

    BEGIN_BLOCK_OBJC_EXCEPTIONS

    // complex code that is risky and can throw an exception

    END_BLOCK_OBJC_EXCEPTIONS

    if (!font)
        font = [NSFont menuFontOfSize:0];
Comment 22 Karolis 2019-10-17 06:06:13 PDT
I'm not sure which Safari version the patch is supposed to land in, but this issue still exists in Safari 13.0.2 on Mojave (early 2015 Mac). However, it works fine in Safari 13.0.2 on Catalina (2017 Mac).
Comment 23 Alexey Proskuryakov 2019-10-17 13:24:18 PDT
This should be fixed in an update that shipped yesterday. It still has the same 13.0.2 version as before, so we need to check build number to confirm.

Could you please check build number via Safari About box? The one with the fix is 14608.2.40.1.3. If you have an older one, the new one can be installed via Software Update.
Comment 24 bunnyhero 2019-12-02 13:32:39 PST
It's still happening to me on Mojave 10.14.6 (18G103), Safari 13.0.3 (14608.3.10.10.1). This website crashes when I click the "Select Combo Tickets" dropdown: https://www.rom.on.ca/en/visit-us/tickets-hours

If I add a custom stylesheet that contains `select { text-rendering: optimizeSpeed !important; }`, the crash does not occur.
Comment 25 Alexey Proskuryakov 2019-12-02 23:29:25 PST
Thank you, I can reproduce. This was fixed in Safari 13.0.2 (14608.2.40.1.3), and it's broken again in Safari 13.0.3.

Keeping this bug in RESOLVED/FIXED state, because the fix is still present in latest WebKit code base. I started a discussion within Apple.