[webkit-reviews] review granted: [Bug 222074] [macOS] Disabling relaunch on login for the WebContent process is racy : [Attachment 420981] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 222074: [macOS] Disabling relaunch on login for the WebContent process is
racy
https://bugs.webkit.org/show_bug.cgi?id=222074

Attachment 420981: Patch

https://bugs.webkit.org/attachment.cgi?id=420981&action=review




--- Comment #10 from Darin Adler <darin at apple.com> ---
Comment on attachment 420981
  --> https://bugs.webkit.org/attachment.cgi?id=420981
Patch

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

> Source/WebKit/ChangeLog:11
> +	   When NSApplication is being intialized, the method -[NSApplication
disableRelaunchOnLogin] is dispatched on a non-main thread, which is in a race
> +	   with the revocation of the Launch Services sandbox extension. This
patch addresses this by setting this information synchronously with Launch
> +	   Services while the sandbox extension is being held.

Given this description I would expect code to be removed as well as code to be
added. Unclear why code is not removed?

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:332
> +    // Disable relaunch on login

WebKit coding style uses a period in a sentence style comment.

A more useful comment would be a little clearer on *why*, maybe even a short
sentence somehow mentions that this must be done synchronously in this
particular place, and why that’s OK from a performance point of view? And if
there is another attempt at this elsewhere that is ineffective, might refer to
that.