Bug 191722 – [Mac] Regression: WebContent process's display name is no longer set

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

WebContent process's display name is no longer set and is thus not reported in Activity Monitor.

[Chris Dumez]

<rdar://problem/45960550>

[Chris Dumez]

Created attachment 354994 [details]
Patch

[Brent Fulgham]

Comment on attachment 354994 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:228
> +        auto error = _LSSetApplicationInformationItem(kLSDefaultSessionID, _LSGetCurrentApplicationASN(), _kLSDisplayNameKey, (CFStringRef)applicationName, nullptr);

As long as this is happening after we enter the sandbox, we should be fine.

Per Arne: Can you help make sure this isn't opening a side channel to WindowServer?

[Chris Dumez]

Comment on attachment 354994 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:353
> +    [NSApplication sharedApplication];

If you'd prefer, it would also work if I called _RegisterApplication(NULL, NULL);
Let me know if you'd prefer that.

[Per Arne Vollan]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 354994 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354994&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:353
> > +    [NSApplication sharedApplication];
> 
> If you'd prefer, it would also work if I called _RegisterApplication(NULL,
> NULL);
> Let me know if you'd prefer that.

Yes, I think that would be better, since we are trying to avoid calling [NSApplication sharedApplication].

[Chris Dumez]

Created attachment 355061 [details]
Patch

[Chris Dumez]

(In reply to Per Arne Vollan from comment #5)
> (In reply to Chris Dumez from comment #4)
> > Comment on attachment 354994 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=354994&action=review
> > 
> > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:353
> > > +    [NSApplication sharedApplication];
> > 
> > If you'd prefer, it would also work if I called _RegisterApplication(NULL,
> > NULL);
> > Let me know if you'd prefer that.
> 
> Yes, I think that would be better, since we are trying to avoid calling
> [NSApplication sharedApplication].

Ok, will re-upload a patch shortly.

[Chris Dumez]

Created attachment 355062 [details]
Patch

[Per Arne Vollan]

Comment on attachment 355062 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:240
> +    dispatch_async(dispatch_get_global_queue(QOS_CLASS_BACKGROUND, 0), ^{
> +        // Note that it is important for [NSApplication sharedApplication] to have been called before setting the display name.
> +        auto error = _LSSetApplicationInformationItem(kLSDefaultSessionID, _LSGetCurrentApplicationASN(), _kLSDisplayNameKey, (CFStringRef)applicationName, nullptr);
> +        ASSERT(!error);
> +        if (error) {
> +            RELEASE_LOG_ERROR(Process, "Failed to set the display name of the WebContent process, error code: %d", error);
> +            return;
> +        }
> +#if !ASSERT_DISABLED
> +        // It is possible for _LSSetApplicationInformationItem() to return 0 and yet fail to set the display name so we make sure the display name has actually been set.
> +        String actualApplicationName = adoptCF((CFStringRef)_LSCopyApplicationInformationItem(kLSDefaultSessionID, _LSGetCurrentApplicationASN(), _kLSDisplayNameKey)).get();
> +        ASSERT(!actualApplicationName.isEmpty());
> +#endif
> +    });

Can this be called synchronously now?

[Chris Dumez]

(In reply to Per Arne Vollan from comment #9)
> Comment on attachment 355062 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355062&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:240
> > +    dispatch_async(dispatch_get_global_queue(QOS_CLASS_BACKGROUND, 0), ^{
> > +        // Note that it is important for [NSApplication sharedApplication] to have been called before setting the display name.
> > +        auto error = _LSSetApplicationInformationItem(kLSDefaultSessionID, _LSGetCurrentApplicationASN(), _kLSDisplayNameKey, (CFStringRef)applicationName, nullptr);
> > +        ASSERT(!error);
> > +        if (error) {
> > +            RELEASE_LOG_ERROR(Process, "Failed to set the display name of the WebContent process, error code: %d", error);
> > +            return;
> > +        }
> > +#if !ASSERT_DISABLED
> > +        // It is possible for _LSSetApplicationInformationItem() to return 0 and yet fail to set the display name so we make sure the display name has actually been set.
> > +        String actualApplicationName = adoptCF((CFStringRef)_LSCopyApplicationInformationItem(kLSDefaultSessionID, _LSGetCurrentApplicationASN(), _kLSDisplayNameKey)).get();
> > +        ASSERT(!actualApplicationName.isEmpty());
> > +#endif
> > +    });
> 
> Can this be called synchronously now?

This is intentional, for performance reasons. This has been measured as costly during process launch and the _LS API is thread-safe.

[Chris Dumez]

(In reply to Chris Dumez from comment #10)
> (In reply to Per Arne Vollan from comment #9)
> > Comment on attachment 355062 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=355062&action=review
> > 
> > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:240
> > > +    dispatch_async(dispatch_get_global_queue(QOS_CLASS_BACKGROUND, 0), ^{
> > > +        // Note that it is important for [NSApplication sharedApplication] to have been called before setting the display name.
> > > +        auto error = _LSSetApplicationInformationItem(kLSDefaultSessionID, _LSGetCurrentApplicationASN(), _kLSDisplayNameKey, (CFStringRef)applicationName, nullptr);
> > > +        ASSERT(!error);
> > > +        if (error) {
> > > +            RELEASE_LOG_ERROR(Process, "Failed to set the display name of the WebContent process, error code: %d", error);
> > > +            return;
> > > +        }
> > > +#if !ASSERT_DISABLED
> > > +        // It is possible for _LSSetApplicationInformationItem() to return 0 and yet fail to set the display name so we make sure the display name has actually been set.
> > > +        String actualApplicationName = adoptCF((CFStringRef)_LSCopyApplicationInformationItem(kLSDefaultSessionID, _LSGetCurrentApplicationASN(), _kLSDisplayNameKey)).get();
> > > +        ASSERT(!actualApplicationName.isEmpty());
> > > +#endif
> > > +    });
> > 
> > Can this be called synchronously now?
> 
> This is intentional, for performance reasons. This has been measured as
> costly during process launch and the _LS API is thread-safe.

In order words, it would work if I did it synchronously but it'd slow process WebProcess initialization by a few milliseconds unnecessarily.

[Per Arne Vollan]

Comment on attachment 355062 [details]
Patch

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

R=me.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:228
> +        // Note that it is important for [NSApplication sharedApplication] to have been called before setting the display name.

Replace [NSApplication sharedApplication] with  _RegisterApplication.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:355
> +    _RegisterApplication(NULL, NULL);

NULL -> nullptr

[Chris Dumez]

Created attachment 355064 [details]
Patch

[Chris Dumez]

Created attachment 355066 [details]
Patch

[Chris Dumez]

Created attachment 355067 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 355067 [details]
Patch

Clearing flags on attachment: 355067

Committed r238289: <https://trac.webkit.org/changeset/238289>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.