Bug 183604 - When the WebContent process is blocked from accessing the WindowServer, the call CVDisplayLinkCreateWithCGDisplay will fail.
Summary: When the WebContent process is blocked from accessing the WindowServer, the c...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks: 183782
  Show dependency treegraph
 
Reported: 2018-03-13 11:41 PDT by Per Arne Vollan
Modified: 2018-03-22 11:42 PDT (History)
7 users (show)

See Also:


Attachments
Patch (24.20 KB, patch)
2018-03-13 11:59 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (24.28 KB, patch)
2018-03-13 12:26 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (24.78 KB, patch)
2018-03-13 12:38 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (24.84 KB, patch)
2018-03-13 13:26 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (24.95 KB, patch)
2018-03-13 14:47 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (24.97 KB, patch)
2018-03-13 15:02 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (26.55 KB, patch)
2018-03-15 14:41 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (26.56 KB, patch)
2018-03-15 16:00 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (26.60 KB, patch)
2018-03-15 16:21 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (3.02 MB, application/zip)
2018-03-15 18:14 PDT, EWS Watchlist
no flags Details
Patch (27.04 KB, patch)
2018-03-16 20:33 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.00 MB, application/zip)
2018-03-16 22:33 PDT, EWS Watchlist
no flags Details
Patch (27.04 KB, patch)
2018-03-17 08:24 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (27.04 KB, patch)
2018-03-19 08:27 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (27.44 KB, patch)
2018-03-19 09:05 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (27.53 KB, patch)
2018-03-19 09:36 PDT, 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-03-13 11:41:01 PDT
When WindowServer access is blocked in the WebContent process, the function call CVDisplayLinkCreateWithCGDisplay in https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/mac/DisplayRefreshMonitorMac.cpp will fail. Should we call this function in the UIProcess, and send a message to the WebContent process for each refresh callback?
Comment 1 Per Arne Vollan 2018-03-13 11:59:25 PDT
Created attachment 335707 [details]
Patch
Comment 2 Per Arne Vollan 2018-03-13 12:26:04 PDT
Created attachment 335710 [details]
Patch
Comment 3 Per Arne Vollan 2018-03-13 12:38:47 PDT
Created attachment 335713 [details]
Patch
Comment 4 Per Arne Vollan 2018-03-13 12:46:38 PDT
This patch fixes the test compositing/animation/animation-backing.html, which fails when the WebContent process does not have WindowServer access.
Comment 5 Per Arne Vollan 2018-03-13 12:48:16 PDT
I will do some measurements to compare the latency of the display update notifications, with and without this patch.

I also think there is room for performance improvements in the current patch.
Comment 6 Per Arne Vollan 2018-03-13 13:26:38 PDT
Created attachment 335720 [details]
Patch
Comment 7 Per Arne Vollan 2018-03-13 14:47:32 PDT
Created attachment 335734 [details]
Patch
Comment 8 Per Arne Vollan 2018-03-13 14:59:21 PDT
The test animations/animation-callback-timestamp.html, which is measuring the performance of requestAnimationFrame, is passing with this patch.
Comment 9 Per Arne Vollan 2018-03-13 15:02:07 PDT
Created attachment 335736 [details]
Patch
Comment 10 Brent Fulgham 2018-03-13 16:30:59 PDT
Comment on attachment 335736 [details]
Patch

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

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

Please show the Radar here.

> Source/WebCore/platform/graphics/DisplayRefreshMonitorManager.cpp:132
> +            monitor->displayLinkFired();

Can more than one monitor have the same displayID? I guess if we have multiple tabs displayed at the same time?

> Source/WebKit/UIProcess/mac/DisplayLink.cpp:38
> +    : m_displayLink(nullptr)

Shouldn't this be initialized in the header?

> Source/WebKit/UIProcess/mac/DisplayLink.cpp:42
> +    if (error)

Seems like we should log that we couldn't make this connection. Might be very helpful in debugging this new behavior.

WTFLogAlways("Could not create a display link: %d", error);

> Source/WebKit/UIProcess/mac/DisplayLink.cpp:45
> +    error = CVDisplayLinkSetOutputCallback(m_displayLink, displayLinkCallback, &webPageProxy);

Ditto.

> Source/WebKit/UIProcess/mac/DisplayLink.cpp:49
> +    error = CVDisplayLinkStart(m_displayLink);

Ditto.

> Source/WebKit/UIProcess/mac/DisplayLink.cpp:56
> +        CVDisplayLinkRelease(m_displayLink);

m_displayLink = nullptr;

> Source/WebKit/UIProcess/mac/DisplayLink.h:47
> +    CVDisplayLinkRef m_displayLink;

CVDisplayLinkRef m_displayLink { nullptr } ?

> Source/WebKit/WebProcess/WebPage/DrawingArea.cpp:92
> +RefPtr<WebCore::DisplayRefreshMonitor> DrawingArea::createDisplayRefreshMonitor(PlatformDisplayID displayID)

Remove 'displayID' here. I think this might cause compile warnings if this was enabled.

> Source/WebKit/WebProcess/WebPage/mac/DrawingAreaMac.cpp:77
> +    m_webPage->send(Messages::WebPageProxy::StartDisplayRefreshMonitor(displayID()));

I'm worried that if there are threading reasons why we don't want to call 'setIsScheduled' outside the lock, we may not want to send this message yet, either. On the other hand, I actually think you don't need these locks.

> Source/WebKit/WebProcess/WebPage/mac/DrawingAreaMac.cpp:79
> +    LockHolder lock(mutex());

I'm not sure these locks are necessary in the WebContent process, since these calls are being triggered by a message sent from the UIProcess. I suspect you needed this previously because the displayLink could fire on some OS interval outside the band of the main thread, but now we hit these on the main message thread.

If we have performance problems, maybe we will need a dedicated (higher priority) message queue, so that these get processed as soon after the display update is requested as possible.
Comment 11 Brent Fulgham 2018-03-13 16:43:59 PDT
Comment on attachment 335736 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/mac/DrawingAreaMac.cpp:87
> +    LockHolder lock(mutex());

I did some debugging, and I'm pretty sure we don't want these, since this is all on a message thread in the WebContent process.

We needed the mutex here when the WindowServer was driving calls to this method; but now this is happening in response to a message from the UIProcess, so I do not believe multiple threads are hitting this code.
Comment 12 Brent Fulgham 2018-03-13 17:10:06 PDT
Comment on attachment 335736 [details]
Patch

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

I think this look good, but the mutex use in the new WebProcess code is unnecessary. Please remove them and just ASSERT that they are always being called on the MainThread.

>> Source/WebKit/WebProcess/WebPage/mac/DrawingAreaMac.cpp:79
>> +    LockHolder lock(mutex());
> 
> I'm not sure these locks are necessary in the WebContent process, since these calls are being triggered by a message sent from the UIProcess. I suspect you needed this previously because the displayLink could fire on some OS interval outside the band of the main thread, but now we hit these on the main message thread.
> 
> If we have performance problems, maybe we will need a dedicated (higher priority) message queue, so that these get processed as soon after the display update is requested as possible.

Yeah -- I just rebuilt with your patch, and we always hit this on the main thread in response to messaging from the UIProcess.

So please add an ASSERT(isMainThread()); here, and remove the lock.

>> Source/WebKit/WebProcess/WebPage/mac/DrawingAreaMac.cpp:87
>> +    LockHolder lock(mutex());
> 
> I did some debugging, and I'm pretty sure we don't want these, since this is all on a message thread in the WebContent process.
> 
> We needed the mutex here when the WindowServer was driving calls to this method; but now this is happening in response to a message from the UIProcess, so I do not believe multiple threads are hitting this code.

Again -- I just rebuilt with your patch, and we always hit this on the main thread in response to messaging from the UIProcess.

So please add an ASSERT(isMainThread()); here, and remove the lock.
Comment 13 Brent Fulgham 2018-03-13 17:10:56 PDT
Note: r- to get rid of those mutexes. Otherwise, I think this looks good -- but I think we need:

(1) Tim Horton or Simon to look at it.
(2) Please confirm Motion Mark looks okay with it.
Comment 14 Simon Fraser (smfr) 2018-03-13 17:46:26 PDT
If we're running the display link the in UI process and the web process is backed up, what happens? Do we queue up a lot of callbacks and handle them all at once, or coalesce?

Also how does this impact rdar://problem/35064753
Comment 15 Per Arne Vollan 2018-03-14 14:07:09 PDT
(In reply to Brent Fulgham from comment #12)
> Comment on attachment 335736 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335736&action=review
> 
> I think this look good, but the mutex use in the new WebProcess code is
> unnecessary. Please remove them and just ASSERT that they are always being
> called on the MainThread.
> 
> >> Source/WebKit/WebProcess/WebPage/mac/DrawingAreaMac.cpp:79
> >> +    LockHolder lock(mutex());
> > 
> > I'm not sure these locks are necessary in the WebContent process, since these calls are being triggered by a message sent from the UIProcess. I suspect you needed this previously because the displayLink could fire on some OS interval outside the band of the main thread, but now we hit these on the main message thread.
> > 
> > If we have performance problems, maybe we will need a dedicated (higher priority) message queue, so that these get processed as soon after the display update is requested as possible.
> 
> Yeah -- I just rebuilt with your patch, and we always hit this on the main
> thread in response to messaging from the UIProcess.
> 
> So please add an ASSERT(isMainThread()); here, and remove the lock.
> 
> >> Source/WebKit/WebProcess/WebPage/mac/DrawingAreaMac.cpp:87
> >> +    LockHolder lock(mutex());
> > 
> > I did some debugging, and I'm pretty sure we don't want these, since this is all on a message thread in the WebContent process.
> > 
> > We needed the mutex here when the WindowServer was driving calls to this method; but now this is happening in response to a message from the UIProcess, so I do not believe multiple threads are hitting this code.
> 
> Again -- I just rebuilt with your patch, and we always hit this on the main
> thread in response to messaging from the UIProcess.
> 
> So please add an ASSERT(isMainThread()); here, and remove the lock.

Thanks for reviewing, Brent! I will add an assert and remove the lock.

Also, this patch seems to introduce a MotionMark regression. I think we need to look into using a dedicated message queue as you mentioned or another IPC method.
Comment 16 Per Arne Vollan 2018-03-14 14:21:50 PDT
(In reply to Simon Fraser (smfr) from comment #14)
> If we're running the display link the in UI process and the web process is
> backed up, what happens? Do we queue up a lot of callbacks and handle them
> all at once, or coalesce?
> 

I believe we coalesce, since we don't dispatch the function to handle the display update on the run loop, unless the previous frame has been handled.

> Also how does this impact rdar://problem/35064753

I will have to look further into this.

Thanks for reviewing, Simon!
Comment 17 Per Arne Vollan 2018-03-15 14:41:08 PDT
Created attachment 335887 [details]
Patch
Comment 18 Per Arne Vollan 2018-03-15 16:00:11 PDT
Created attachment 335901 [details]
Patch
Comment 19 Per Arne Vollan 2018-03-15 16:21:47 PDT
Created attachment 335907 [details]
Patch
Comment 20 Per Arne Vollan 2018-03-15 17:03:55 PDT
I think it is unneeded to send the display ID over IPC to the UI process, since the display ID is already available there. I will update the patch.
Comment 21 EWS Watchlist 2018-03-15 18:14:44 PDT
Comment on attachment 335907 [details]
Patch

Attachment 335907 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6973729

New failing tests:
fast/canvas/webgl/texImage2D-video-flipY-true.html
fast/canvas/webgl/texImage2D-video-flipY-false.html
Comment 22 EWS Watchlist 2018-03-15 18:14:45 PDT
Created attachment 335918 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 23 Per Arne Vollan 2018-03-16 10:19:13 PDT
(In reply to Build Bot from comment #21)
> Comment on attachment 335907 [details]
> Patch
> 
> Attachment 335907 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.webkit.org/results/6973729
> 
> New failing tests:
> fast/canvas/webgl/texImage2D-video-flipY-true.html
> fast/canvas/webgl/texImage2D-video-flipY-false.html

I am currently looking into these test failures.
Comment 24 Per Arne Vollan 2018-03-16 20:33:10 PDT
Created attachment 335996 [details]
Patch
Comment 25 EWS Watchlist 2018-03-16 22:33:22 PDT
Comment on attachment 335996 [details]
Patch

Attachment 335996 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6994572

New failing tests:
http/tests/preload/download_resources.html
Comment 26 EWS Watchlist 2018-03-16 22:33:33 PDT
Created attachment 335999 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 27 Per Arne Vollan 2018-03-17 08:24:42 PDT
Created attachment 336007 [details]
Patch
Comment 28 Brent Fulgham 2018-03-18 14:42:56 PDT
Comment on attachment 336007 [details]
Patch

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

Looks good. Please keep an eye on the performance bots to make sure we don't regress anything.

> Source/WebCore/ChangeLog:10
> +        about display updates, by sending a message from the UIProcess on each screen update. This patch adds an extra

No comma needed here: "... about display updates by sending a message..."

> Source/WebKit/UIProcess/mac/DisplayLink.cpp:48
> +        WTFLogAlways("Could not create a display link: %d", error);

"Could not set the display link output callback: %d"

> Source/WebKit/UIProcess/mac/DisplayLink.cpp:54
> +        WTFLogAlways("Could not create a display link: %d", error);

"Could not start the display link: %d"
Comment 29 Per Arne Vollan 2018-03-19 08:27:16 PDT
Created attachment 336045 [details]
Patch
Comment 30 Per Arne Vollan 2018-03-19 09:05:41 PDT
Created attachment 336046 [details]
Patch
Comment 31 Per Arne Vollan 2018-03-19 09:31:18 PDT
(In reply to Brent Fulgham from comment #28)
> Comment on attachment 336007 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336007&action=review
> 
> Looks good. Please keep an eye on the performance bots to make sure we don't
> regress anything.
> 
> > Source/WebCore/ChangeLog:10
> > +        about display updates, by sending a message from the UIProcess on each screen update. This patch adds an extra
> 
> No comma needed here: "... about display updates by sending a message..."
> 
> > Source/WebKit/UIProcess/mac/DisplayLink.cpp:48
> > +        WTFLogAlways("Could not create a display link: %d", error);
> 
> "Could not set the display link output callback: %d"
> 
> > Source/WebKit/UIProcess/mac/DisplayLink.cpp:54
> > +        WTFLogAlways("Could not create a display link: %d", error);
> 
> "Could not start the display link: %d"

Thanks for reviewing! I will update the patch before landing.
Comment 32 Per Arne Vollan 2018-03-19 09:36:32 PDT
Created attachment 336047 [details]
Patch
Comment 33 Per Arne Vollan 2018-03-19 09:47:41 PDT
Also, I was wondering if we really need to ask the system (with a display link) for notifications about display updates. Shouldn't we be able to determine ourselves if we have updated the display? I assume we're only interested in updates in our own window. Perhaps it is difficult to determine display updates when we're running a CA layer animation? If it is possible to detect display updates ourselves, we could add a run loop observer at the end of the run loop to detect display updates, and then notify the display refresh monitors on the next iteration of the run loop. This is a potential follow-up patch. What do you think?
Comment 34 Tim Horton 2018-03-19 10:11:57 PDT
This mechanism is for synchronizing our work with the refresh rate and timing of the display, which are both things that only the system knows.
Comment 35 Per Arne Vollan 2018-03-19 10:41:36 PDT
(In reply to Tim Horton from comment #34)
> This mechanism is for synchronizing our work with the refresh rate and
> timing of the display, which are both things that only the system knows.

I see, thanks Tim!
Comment 36 WebKit Commit Bot 2018-03-19 11:04:44 PDT
Comment on attachment 336047 [details]
Patch

Clearing flags on attachment: 336047

Committed r229707: <https://trac.webkit.org/changeset/229707>
Comment 37 Simon Fraser (smfr) 2018-03-19 11:21:35 PDT
Comment on attachment 336047 [details]
Patch

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

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:612
> +        uint32_t displayID = [[[[platformWindow() screen] deviceDescription] objectForKey:@"NSScreenNumber"] intValue];

This seems like a really weird way to get the display ID.

What happens when this window moves to a different screen?
Comment 38 Per Arne Vollan 2018-03-19 13:55:00 PDT
(In reply to Simon Fraser (smfr) from comment #37)
> Comment on attachment 336047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336047&action=review
> 
> > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:612
> > +        uint32_t displayID = [[[[platformWindow() screen] deviceDescription] objectForKey:@"NSScreenNumber"] intValue];
> 
> This seems like a really weird way to get the display ID.
>

Yes, indeed. I think it would have been better to use the function 'displayID(NSScreen *screen)' in PlatformScreenMac.mm, which encapsulates this.
 
> What happens when this window moves to a different screen?

I believe this should work. The UIProcess will then send a message to the WebContent process, notifying it about the new display ID. All display refresh monitors will then be deleted, which will delete the display link object in the UI process. Then, new monitors with the updated display ID will be created, and a new display link object will be created in the UI process with the new display ID. This is based on code inspection. I will need to test this more carefully.

Thanks for reviewing!
Comment 39 Tim Horton 2018-03-19 21:42:37 PDT
Comment on attachment 336047 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/mac/DrawingAreaMac.cpp:43
> +class DisplayRefreshMonitorMac : public DisplayRefreshMonitor {

It’s super confusing that we have WebCore::DisplayRefreshMonitorMac and WebKit::DisplayRefreshMonitorMac, which are completely different implementations of the same thing, with the same name (and which both are included in some builds??). That’s very unusual. And will be confusing in the future. Also weird that you created this new file, when there’s only one Mac DrawingArea subclass (TiledCoreAnimationDrawingArea). Could have just stuck it there.

It looks like you were following the example of RemoteLayerTree stuff, so this might all be my fault in the end. 😶
Comment 40 Per Arne Vollan 2018-03-20 10:11:04 PDT
(In reply to Tim Horton from comment #39)
> Comment on attachment 336047 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336047&action=review
> 
> > Source/WebKit/WebProcess/WebPage/mac/DrawingAreaMac.cpp:43
> > +class DisplayRefreshMonitorMac : public DisplayRefreshMonitor {
> 
> It’s super confusing that we have WebCore::DisplayRefreshMonitorMac and
> WebKit::DisplayRefreshMonitorMac, which are completely different
> implementations of the same thing, with the same name (and which both are
> included in some builds??). That’s very unusual. And will be confusing in
> the future. Also weird that you created this new file, when there’s only one
> Mac DrawingArea subclass (TiledCoreAnimationDrawingArea). Could have just
> stuck it there.
> 
> It looks like you were following the example of RemoteLayerTree stuff, so
> this might all be my fault in the end. 😶

Thanks for reviewing! I think I will create a follow-up patch to address the review comments :)
Comment 41 Brent Fulgham 2018-03-22 11:41:54 PDT
Comment on attachment 336007 [details]
Patch

Clearing review flag.
Comment 42 Brent Fulgham 2018-03-22 11:42:36 PDT
<rdar://problem/38305109>