Bug 222074 – [macOS] Disabling relaunch on login for the WebContent process is racy

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Delay revoking sandbox extension to Launch Services until Web page is created, since revoking it when initializing the Web process is too early. AppKit may access the daemon after that.

[Per Arne Vollan]

<rdar://74230216>

[Per Arne Vollan]

Created attachment 420719 [details]
Patch

[Geoffrey Garen]

I think the logic here is that +[NSAccessibilityRemoteUIElement remoteTokenForLocalUIElement:] does a one-time call to LaunchServices, and we want to make sure to make that call before closing the sandbox.

I feel like the logic might be more clear if we just wrote it outright: In WebProcess::platformInitializeWebProcess(), can you explicitly call [NSAccessibilityRemoteUIElement remoteTokenForLocalUIElement:[[[WKAccessibilityWebPageObject alloc] init] autorelease]], and then revoke? That would feel more straightforward and self-documenting.

But also: What does this revocation achieve? After we have made the connection to LaunchServices, what action severs that connection?

[Per Arne Vollan]

Created attachment 420834 [details]
Patch

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #3)
> I think the logic here is that +[NSAccessibilityRemoteUIElement
> remoteTokenForLocalUIElement:] does a one-time call to LaunchServices, and
> we want to make sure to make that call before closing the sandbox.
> 
> I feel like the logic might be more clear if we just wrote it outright: In
> WebProcess::platformInitializeWebProcess(), can you explicitly call
> [NSAccessibilityRemoteUIElement
> remoteTokenForLocalUIElement:[[[WKAccessibilityWebPageObject alloc] init]
> autorelease]], and then revoke? That would feel more straightforward and
> self-documenting.
> 


From having a breakpoint in the debugger when the connection to Launch Services is being used in WebProcess::platformInitializeWebProcess, it appears that the Launch Services is being accessed on a non-main thread created by dispatch_async of the method -[NSApplication disableRelaunchOnLogin] which is being called when NSApplication is being initialized.

It might be sufficient to revoke and sever the connection from a RunLoop::main dispatch, but it is possibly not guaranteed to happen after the dispatch_async. Would you prefer doing this with a RunLoop::main dispatch instead?

> But also: What does this revocation achieve? After we have made the
> connection to LaunchServices, what action severs that connection?

Ah, great catch! When we delay the revocation of the extension, we should also delay the severing of the Launch Services connection, which is done by calling _LSSetApplicationLaunchServicesServerConnectionStatus with a specified set of flags. This has been fixed in the latest patch.

Thanks for reviewing!

[Geoffrey Garen]

> From having a breakpoint in the debugger when the connection to Launch
> Services is being used in WebProcess::platformInitializeWebProcess, it
> appears that the Launch Services is being accessed on a non-main thread
> created by dispatch_async of the method -[NSApplication
> disableRelaunchOnLogin] which is being called when NSApplication is being
> initialized.
> 
> It might be sufficient to revoke and sever the connection from a
> RunLoop::main dispatch, but it is possibly not guaranteed to happen after
> the dispatch_async. Would you prefer doing this with a RunLoop::main
> dispatch instead?

Hmmm... 

Does this mean that this patch as-is is a race against a dispatch_async, and if/when the dispatch_async fires second, it will attempt to use a closed connection to LaunchServices, and crash?

Seems like we need some kind of explicit synchronization to fix that. Or we need LaunchServices to fail silently instead of crashing.

RunLoop::main dispatch would not synchronize with a dispatch_async to an unrelated queue. So, I don't think RunLoop::main dispatch would resolve a race condition.

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #6)
> > From having a breakpoint in the debugger when the connection to Launch
> > Services is being used in WebProcess::platformInitializeWebProcess, it
> > appears that the Launch Services is being accessed on a non-main thread
> > created by dispatch_async of the method -[NSApplication
> > disableRelaunchOnLogin] which is being called when NSApplication is being
> > initialized.
> > 
> > It might be sufficient to revoke and sever the connection from a
> > RunLoop::main dispatch, but it is possibly not guaranteed to happen after
> > the dispatch_async. Would you prefer doing this with a RunLoop::main
> > dispatch instead?
> 
> Hmmm... 
> 
> Does this mean that this patch as-is is a race against a dispatch_async, and
> if/when the dispatch_async fires second, it will attempt to use a closed
> connection to LaunchServices, and crash?
> 

Yes.

> Seems like we need some kind of explicit synchronization to fix that. Or we
> need LaunchServices to fail silently instead of crashing.
> 

That is a good point. What if we disable relaunch on login synchronously while holding the Launch Services extension, and then have Launch Services fail silently? 

> RunLoop::main dispatch would not synchronize with a dispatch_async to an
> unrelated queue. So, I don't think RunLoop::main dispatch would resolve a
> race condition.

Yes, that makes sense.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 420980 [details]
Patch

[Per Arne Vollan]

Created attachment 420981 [details]
Patch

[Darin Adler]

Comment on attachment 420981 [details]
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.

[Geoffrey Garen]

> That is a good point. What if we disable relaunch on login synchronously
> while holding the Launch Services extension, and then have Launch Services
> fail silently?

I think this can work.

But what about the NSAccessibilityRemoteUIElement case? (See crash log attached to rdar://problem/74230216.)

[Per Arne Vollan]

(In reply to Darin Adler from comment #10)
> Comment on attachment 420981 [details]
> 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?
> 

This is also being done from -[NSApplication init] by dispatching -[NSApplication disableRelaunchOnLogin] on a non-main thread.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:332
> > +    // Disable relaunch on login
> 
> WebKit coding style uses a period in a sentence style comment.
> 

Will fix!

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

Will fix in upcoming patch.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #11)
> > That is a good point. What if we disable relaunch on login synchronously
> > while holding the Launch Services extension, and then have Launch Services
> > fail silently?
> 
> I think this can work.
> 
> But what about the NSAccessibilityRemoteUIElement case? (See crash log
> attached to rdar://problem/74230216.)

Ah, great catch, I completely missed that one! I will add the code you suggested from a previous comment.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 421059 [details]
Patch

[Per Arne Vollan]

Created attachment 421091 [details]
Patch

[Darin Adler]

Comment on attachment 421091 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:339
> +    [NSAccessibilityRemoteUIElement remoteTokenForLocalUIElement:[[[WKAccessibilityWebPageObject alloc] init] autorelease]];

Can minimize the use of autorelease here, which is something Chris Dumez is working on across WebKit. Write this, which uses RetainPtr instead:

    [NSAccessibilityRemoteUIElement remoteTokenForLocalUIElement:adoptNS([[WKAccessibilityWebPageObject alloc] init]).get()];

[Per Arne Vollan]

Created attachment 421106 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #16)
> Comment on attachment 421091 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421091&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:339
> > +    [NSAccessibilityRemoteUIElement remoteTokenForLocalUIElement:[[[WKAccessibilityWebPageObject alloc] init] autorelease]];
> 
> Can minimize the use of autorelease here, which is something Chris Dumez is
> working on across WebKit. Write this, which uses RetainPtr instead:
> 
>     [NSAccessibilityRemoteUIElement
> remoteTokenForLocalUIElement:adoptNS([[WKAccessibilityWebPageObject alloc]
> init]).get()];

Fixed, I have uploaded a new patch for review.

Thanks for reviewing!

[Geoffrey Garen]

Comment on attachment 421106 [details]
Patch

r=me

[EWS]

Committed r273270: <https://commits.webkit.org/r273270>

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

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #19)
> Comment on attachment 421106 [details]
> Patch
> 
> r=me

Thank you!