Bug 202198

Summary:

REGRESSION (r245672): <select> dropdown with text-rendering: optimizeLegibility freezes Safari

Product:

WebKit

Reporter:

yann

Component:

Text

Assignee:

Myles C. Maxfield <mmaxfield>

Status:

RESOLVED FIXED

Severity:

Critical

CC:

achristensen, andrea, ap, benjamin, bunnyhero, cdumez, cmarcelo, commit-queue, darin, dbates, dino, ews-watchlist, jonlee, karolis.n, matthewwithanm, mmaxfield, nilfgard, simon.fraser, thorton, webkit-bug-importer, yann

Priority:

Keywords:

InRadar

Version:

Other

Hardware:

Mac

OS:

macOS 10.14

See Also:

https://bugs.webkit.org/show_bug.cgi?id=202556
https://bugs.webkit.org/show_bug.cgi?id=202262
https://bugs.webkit.org/show_bug.cgi?id=204138

Attachments:

Description	Flags
Patch	none
Patch	thorton: review+
Patch for committing	none
Patch for committing	none

yann

Reported 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>

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2019-09-25 15:17:16 PDT

I cannot reproduce this. Could you please attach a complete crash log?

yann

Comment 2 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...).

Alexey Proskuryakov

Comment 3 2019-09-26 08:54:47 PDT

Thank you! rdar://problem/55744833

Alexey Proskuryakov

Comment 4 2019-09-29 15:22:05 PDT

*** Bug 202262 has been marked as a duplicate of this bug. ***

Myles C. Maxfield

Comment 5 2019-10-01 12:19:53 PDT

I can reproduce this.

Myles C. Maxfield

Comment 6 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.

Myles C. Maxfield

Comment 7 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; }

Myles C. Maxfield

Comment 8 2019-10-01 15:15:48 PDT

NSCTFontOpticalSizeAttribute is the field that's tripping up NSFont.

Myles C. Maxfield

Comment 9 2019-10-01 15:19:45 PDT

Caused by r245672.

Myles C. Maxfield

Comment 10 2019-10-01 15:20:48 PDT

<rdar://problem/55744833>

Myles C. Maxfield

Comment 11 2019-10-01 19:18:54 PDT

Created attachment 379981 [details] Patch

Myles C. Maxfield

Comment 12 2019-10-01 19:23:27 PDT

Created attachment 379982 [details] Patch

Tim Horton

Comment 13 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.

Myles C. Maxfield

Comment 14 2019-10-02 11:30:53 PDT

Created attachment 380042 [details] Patch for committing

Myles C. Maxfield

Comment 15 2019-10-02 15:10:12 PDT

Created attachment 380063 [details] Patch for committing

WebKit Commit Bot

Comment 16 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>

Alexey Proskuryakov

Comment 17 2019-10-03 10:45:37 PDT

Does this fix address the dupe (bug 202262)?

Myles C. Maxfield

Comment 18 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

Darin Adler

Comment 19 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?

Darin Adler

Comment 20 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.

Darin Adler

Comment 21 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];

Karolis

Comment 22 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).

Alexey Proskuryakov

Comment 23 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.

bunnyhero

Comment 24 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.

Alexey Proskuryakov

Comment 25 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.

Sam Sneddon [:gsnedders]

Comment 26 2021-03-25 16:49:50 PDT

*** Bug 202055 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.