Bug 182747 - Create CA layer contexts with +remoteContextWithOptions.
Summary: Create CA layer contexts with +remoteContextWithOptions.
Status: NEW
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: 182855 183048
Blocks: 183663
  Show dependency treegraph
 
Reported: 2018-02-13 14:47 PST by Per Arne Vollan
Modified: 2018-03-22 11:50 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.34 KB, patch)
2018-02-13 14:51 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (3.53 KB, patch)
2018-02-13 15:46 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (3.85 KB, patch)
2018-02-13 16:08 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (3.85 KB, patch)
2018-02-13 18:19 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (5.14 KB, patch)
2018-02-14 09:45 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (5.35 KB, patch)
2018-02-14 10:54 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (5.36 KB, patch)
2018-02-14 11:28 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (9.35 KB, patch)
2018-03-07 16:13 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (8.34 KB, patch)
2018-03-08 14:02 PST, Per Arne Vollan
bfulgham: review+
Details | Formatted Diff | Diff
Patch (5.32 KB, patch)
2018-03-09 13:55 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 2018-02-13 14:47:28 PST
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.
Comment 1 Per Arne Vollan 2018-02-13 14:51:10 PST
Created attachment 333729 [details]
Patch
Comment 2 Brent Fulgham 2018-02-13 15:28:48 PST
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.
Comment 3 Per Arne Vollan 2018-02-13 15:36:27 PST
(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.
Comment 4 Per Arne Vollan 2018-02-13 15:46:02 PST
Created attachment 333738 [details]
Patch
Comment 5 Simon Fraser (smfr) 2018-02-13 15:49:30 PST
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.
Comment 6 Per Arne Vollan 2018-02-13 16:08:20 PST
Created attachment 333740 [details]
Patch
Comment 7 Per Arne Vollan 2018-02-13 16:09:35 PST
(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.
Comment 8 Brent Fulgham 2018-02-13 17:15:57 PST
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.
Comment 9 Tim Horton 2018-02-13 17:19:00 PST
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.
Comment 10 Per Arne Vollan 2018-02-13 18:19:26 PST
Created attachment 333756 [details]
Patch
Comment 11 Per Arne Vollan 2018-02-13 18:20:22 PST
(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.
Comment 12 Brent Fulgham 2018-02-14 08:45:07 PST
(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?
Comment 13 Per Arne Vollan 2018-02-14 09:06:17 PST
(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.
Comment 14 Per Arne Vollan 2018-02-14 09:45:44 PST
Created attachment 333808 [details]
Patch
Comment 15 Simon Fraser (smfr) 2018-02-14 09:46:55 PST
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?
Comment 16 Per Arne Vollan 2018-02-14 09:47:28 PST
(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.
Comment 17 Per Arne Vollan 2018-02-14 09:49:05 PST
(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.
Comment 18 Brent Fulgham 2018-02-14 09:49:58 PST
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.
Comment 19 Per Arne Vollan 2018-02-14 10:17:29 PST
(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.
Comment 20 Brent Fulgham 2018-02-14 10:20:40 PST
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.
Comment 21 Per Arne Vollan 2018-02-14 10:54:51 PST
Created attachment 333818 [details]
Patch
Comment 22 Brent Fulgham 2018-02-14 11:25:51 PST
<rdar://problem/37517466>
Comment 23 Per Arne Vollan 2018-02-14 11:28:53 PST
Created attachment 333822 [details]
Patch
Comment 24 Tim Horton 2018-02-14 11:30:49 PST
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?
Comment 25 Per Arne Vollan 2018-02-14 11:34:03 PST
(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.
Comment 26 Per Arne Vollan 2018-03-07 16:13:30 PST
Created attachment 335238 [details]
Patch
Comment 27 Per Arne Vollan 2018-03-08 14:02:16 PST
Created attachment 335341 [details]
Patch
Comment 28 Brent Fulgham 2018-03-09 13:21:27 PST
Comment on attachment 335341 [details]
Patch

Great -- this looks ready to go!
Comment 29 Per Arne Vollan 2018-03-09 13:55:50 PST
Created attachment 335457 [details]
Patch
Comment 30 Per Arne Vollan 2018-03-09 13:57:31 PST
Comment on attachment 335457 [details]
Patch

Thanks for reviewing!
Comment 31 WebKit Commit Bot 2018-03-09 14:43:54 PST
Comment on attachment 335457 [details]
Patch

Clearing flags on attachment: 335457

Committed r229484: <https://trac.webkit.org/changeset/229484>
Comment 32 Brent Fulgham 2018-03-22 11:50:16 PDT
This is:

<rdar://problem/37517466>