Bug 221088 - [macOS] Crash when updating color preferences
Summary: [macOS] Crash when updating color preferences
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-28 08:19 PST by Per Arne Vollan
Modified: 2021-01-28 10:01 PST (History)
2 users (show)

See Also:


Attachments
Patch (5.01 KB, patch)
2021-01-28 08:35 PST, Per Arne Vollan
bfulgham: review+
Details | Formatted Diff | Diff
Patch (5.46 KB, patch)
2021-01-28 09:32 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2021-01-28 08:19:07 PST
After <https://trac.webkit.org/changeset/271965/webkit>, the WebContent process is no longer allowed to connect to the Launch Services daemon. This introduced a crash in the WebContent process when changing color preferences, since AppKit will then attempt to set application information with Launch Services, which causes a crash when that fails.
Comment 1 Radar WebKit Bug Importer 2021-01-28 08:19:26 PST
<rdar://problem/73709142>
Comment 2 Per Arne Vollan 2021-01-28 08:35:16 PST
Created attachment 418644 [details]
Patch
Comment 3 Brent Fulgham 2021-01-28 09:17:05 PST
Comment on attachment 418644 [details]
Patch

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

r=me, though I think a comment or changed function name might be helpful.

> Source/WebKit/ChangeLog:12
> +        WebContent process.

Do you mean UIProcess here? I think the preference updates are expected in UIProcess, and we should never attempt to make such updates in the WCP.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:188
> +static void updateCanQuitQuietlyAndSafely(NSApplication*, SEL)

Perhaps this would be clearer with a better name, rather than a comment like I suggested below:

static void preventAppKitFromContactingLaunchServicesFromWebContentProcess(NSApplication*, SEL)

... or maybe you can think of a better name.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:189
> +{

Perhaps a comment here would be useful:

// WebKit prohibits communication with Launch Services after entering the sandbox. This method override prevents AppKit
// in the WebContent process from attempting to set color preferences. Color preferences are instead handled in the UIProcess.
Comment 4 Per Arne Vollan 2021-01-28 09:32:20 PST
Created attachment 418651 [details]
Patch
Comment 5 Per Arne Vollan 2021-01-28 09:39:07 PST
(In reply to Brent Fulgham from comment #3)
> Comment on attachment 418644 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418644&action=review
> 
> r=me, though I think a comment or changed function name might be helpful.
> 
> > Source/WebKit/ChangeLog:12
> > +        WebContent process.
> 
> Do you mean UIProcess here? I think the preference updates are expected in
> UIProcess, and we should never attempt to make such updates in the WCP.
> 

This happened when updating color preferences from System Preferences. I updated the change log.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:188
> > +static void updateCanQuitQuietlyAndSafely(NSApplication*, SEL)
> 
> Perhaps this would be clearer with a better name, rather than a comment like
> I suggested below:
> 
> static void
> preventAppKitFromContactingLaunchServicesFromWebContentProcess(NSApplication*
> , SEL)
> 
> ... or maybe you can think of a better name.
> 

Fixed!

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:189
> > +{
> 
> Perhaps a comment here would be useful:
> 
> // WebKit prohibits communication with Launch Services after entering the
> sandbox. This method override prevents AppKit
> // in the WebContent process from attempting to set color preferences. Color
> preferences are instead handled in the UIProcess.

Done, with a minor modification.

Thanks for reviewing!
Comment 6 EWS 2021-01-28 10:01:30 PST
Committed r272017: <https://trac.webkit.org/changeset/272017>

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