Bug 182747 – Create CA layer contexts with +remoteContextWithOptions.

• Status: NEW
• Component: WebKit Misc.

[Per Arne Vollan]

CA layer contexts (CAContext) are currently created with +contextWithCGSConnection, which is using the main WindowServer connection to create the context. Instead, the contexts can be created with +remoteContextWithOptions, which does not use the main WindowServer connection. This is a step towards limiting the access the WebContent process has to the window server.

[Per Arne Vollan]

Created attachment 333729 [details]
Patch

[Brent Fulgham]

Comment on attachment 333729 [details]
Patch

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

> Source/WebKit/Platform/mac/LayerHostingContext.mm:68
> +    layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior :  @"ignore"}];

I think this new behavior needs to be gated on a version check. This might need to be added to the SPI header declarations. Or is this code path 101400 and beyond? Also, why are we using different options than iOS? Should they be more similar?

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:591
> +       (global-name "com.apple.CARenderServer")

I've started adding comments to explain why these are needed. I'd add "    ; Needed for [CAContext remoteContextWithOptions]" at the end of the line.

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #2)
> Comment on attachment 333729 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333729&action=review
> 
> > Source/WebKit/Platform/mac/LayerHostingContext.mm:68
> > +    layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior :  @"ignore"}];
> 
> I think this new behavior needs to be gated on a version check. This might
> need to be added to the SPI header declarations. Or is this code path 101400
> and beyond? Also, why are we using different options than iOS? Should they
> be more similar?
> 
> > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:591
> > +       (global-name "com.apple.CARenderServer")
> 
> I've started adding comments to explain why these are needed. I'd add "    ;
> Needed for [CAContext remoteContextWithOptions]" at the end of the line.

Thanks for reviewing! I'll update the patch.

[Per Arne Vollan]

Created attachment 333738 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 333738 [details]
Patch

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

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=182747

Needs a radar number.

[Per Arne Vollan]

Created attachment 333740 [details]
Patch

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #5)
> Comment on attachment 333738 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333738&action=review
> 
> > Source/WebKit/ChangeLog:4
> > +        https://bugs.webkit.org/show_bug.cgi?id=182747
> 
> Needs a radar number.

Thanks! I also expanded the changelog entry a little.

[Brent Fulgham]

Comment on attachment 333740 [details]
Patch

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

Looks good!

> Source/WebKit/Platform/mac/LayerHostingContext.mm:68
> +    layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior :  @"ignore"}];

Do we need to include 'kCAContextIgnoresHitTest" or "kCAContextDisplayId"? Maybe smfr knows.

[Tim Horton]

Comment on attachment 333740 [details]
Patch

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

> Source/WebKit/Platform/mac/LayerHostingContext.mm:67
> +    [CAContext setAllowsCGSConnections: false];

No space after the colon, and this is probably a BOOL so it should be NO not false, right?

>> Source/WebKit/Platform/mac/LayerHostingContext.mm:68
>> +    layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior :  @"ignore"}];
> 
> Do we need to include 'kCAContextIgnoresHitTest" or "kCAContextDisplayId"? Maybe smfr knows.

No, those are for iOS. This is more like the one below it.

[Per Arne Vollan]

Created attachment 333756 [details]
Patch

[Per Arne Vollan]

(In reply to Tim Horton from comment #9)
> Comment on attachment 333740 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333740&action=review
> 
> > Source/WebKit/Platform/mac/LayerHostingContext.mm:67
> > +    [CAContext setAllowsCGSConnections: false];
> 
> No space after the colon, and this is probably a BOOL so it should be NO not
> false, right?
> 
> >> Source/WebKit/Platform/mac/LayerHostingContext.mm:68
> >> +    layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior :  @"ignore"}];
> > 
> > Do we need to include 'kCAContextIgnoresHitTest" or "kCAContextDisplayId"? Maybe smfr knows.
> 
> No, those are for iOS. This is more like the one below it.

Thanks! I have updated the patch.

[Brent Fulgham]

(In reply to Per Arne Vollan from comment #11)
> (In reply to Tim Horton from comment #9)
> > Comment on attachment 333740 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=333740&action=review
> > 
> > > Source/WebKit/Platform/mac/LayerHostingContext.mm:67
> > > +    [CAContext setAllowsCGSConnections: false];
> > 
> > No space after the colon, and this is probably a BOOL so it should be NO not
> > false, right?
> > 
> > >> Source/WebKit/Platform/mac/LayerHostingContext.mm:68
> > >> +    layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior :  @"ignore"}];
> > > 
> > > Do we need to include 'kCAContextIgnoresHitTest" or "kCAContextDisplayId"? Maybe smfr knows.
> > 
> > No, those are for iOS. This is more like the one below it.
> 
> Thanks! I have updated the patch.

Were you planning on more changes? Or is this up for review now?

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #12)
> (In reply to Per Arne Vollan from comment #11)
> > (In reply to Tim Horton from comment #9)
> > > Comment on attachment 333740 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=333740&action=review
> > > 
> > > > Source/WebKit/Platform/mac/LayerHostingContext.mm:67
> > > > +    [CAContext setAllowsCGSConnections: false];
> > > 
> > > No space after the colon, and this is probably a BOOL so it should be NO not
> > > false, right?
> > > 
> > > >> Source/WebKit/Platform/mac/LayerHostingContext.mm:68
> > > >> +    layerHostingContext->m_context = [CAContext remoteContextWithOptions:@{kCAContextCIFilterBehavior :  @"ignore"}];
> > > > 
> > > > Do we need to include 'kCAContextIgnoresHitTest" or "kCAContextDisplayId"? Maybe smfr knows.
> > > 
> > > No, those are for iOS. This is more like the one below it.
> > 
> > Thanks! I have updated the patch.
> 
> Were you planning on more changes? Or is this up for review now?

I have one more small change, I'll upload it soon.

[Per Arne Vollan]

Created attachment 333808 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 333808 [details]
Patch

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

> Source/WebKit/WebProcess/WebProcess.cpp:226
> +    CGSShutdownServerConnections();

Are there connections to shut down? Why would we have made any before this? Should we just assert that there are none?

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #14)
> Created attachment 333808 [details]
> Patch

The patch seems to introduce some layout tests failures on 10.14, e.g. compositing/animation/animation-backing.html. I will look into it.

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #15)
> Comment on attachment 333808 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333808&action=review
> 
> > Source/WebKit/WebProcess/WebProcess.cpp:226
> > +    CGSShutdownServerConnections();
> 
> Are there connections to shut down? Why would we have made any before this?
> Should we just assert that there are none?

Thanks, good point! Debugging shows that there are no connections at this point, I will add an assert.

[Brent Fulgham]

Comment on attachment 333808 [details]
Patch

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

> Source/WebKit/Shared/mac/HangDetectionDisablerMac.mm:51
> +    // In this case, there will be no valid WindowServer main connection.

If that's the case, should we ASSERT(!CGSMainConnectionID()) and just return, and have the CGSSetConnectionProperty be in an #else clause?

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:590
>         (global-name "com.apple.windowserver.active")

Do we need "windowserver.active"? I can't remember if you found that this endpoint was needed when using the CARenderServer.

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #18)
> Comment on attachment 333808 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333808&action=review
> 
> > Source/WebKit/Shared/mac/HangDetectionDisablerMac.mm:51
> > +    // In this case, there will be no valid WindowServer main connection.
> 
> If that's the case, should we ASSERT(!CGSMainConnectionID()) and just
> return, and have the CGSSetConnectionProperty be in an #else clause?
> 

I think we still need CGSSetConnectionProperty, since this is shared code that might be executed by processes other than the WebContent process. Or am I mistaken?

> > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:590
> >         (global-name "com.apple.windowserver.active")
> 
> Do we need "windowserver.active"? I can't remember if you found that this
> endpoint was needed when using the CARenderServer.

Thanks! I believe you are correct, it is probably not needed. I'll update the patch.

[Brent Fulgham]

Comment on attachment 333808 [details]
Patch

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

>>> Source/WebKit/Shared/mac/HangDetectionDisablerMac.mm:51
>>> +    // In this case, there will be no valid WindowServer main connection.
>> 
>> If that's the case, should we ASSERT(!CGSMainConnectionID()) and just return, and have the CGSSetConnectionProperty be in an #else clause?
> 
> I think we still need CGSSetConnectionProperty, since this is shared code that might be executed by processes other than the WebContent process. Or am I mistaken?

Hmm. I'll bet you are right. This is probably needed for PluginProcess.

[Per Arne Vollan]

Created attachment 333818 [details]
Patch

[Brent Fulgham]

<rdar://problem/37517466>

[Per Arne Vollan]

Created attachment 333822 [details]
Patch

[Tim Horton]

This might deserve at least a cursory glance at some graphics benchmark numbers to make sure the architecture change doesn’t have any impact. Can you do that? Maybe MotionMark?

[Per Arne Vollan]

(In reply to Tim Horton from comment #24)
> This might deserve at least a cursory glance at some graphics benchmark
> numbers to make sure the architecture change doesn’t have any impact. Can
> you do that? Maybe MotionMark?

Absolutely, I'll look into that.

[Per Arne Vollan]

Created attachment 335238 [details]
Patch

[Per Arne Vollan]

Created attachment 335341 [details]
Patch

[Brent Fulgham]

Comment on attachment 335341 [details]
Patch

Great -- this looks ready to go!

[Per Arne Vollan]

Created attachment 335457 [details]
Patch

[Per Arne Vollan]

Comment on attachment 335457 [details]
Patch

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 335457 [details]
Patch

Clearing flags on attachment: 335457

Committed r229484: <https://trac.webkit.org/changeset/229484>

[Brent Fulgham]

This is:

<rdar://problem/37517466>