Bug 183604 – When the WebContent process is blocked from accessing the WindowServer, the call CVDisplayLinkCreateWithCGDisplay will fail.

• Component: WebKit2
• Status: RESOLVED FIXED

[Per Arne Vollan]

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?

[Per Arne Vollan]

Created attachment 335707 [details]
Patch

[Per Arne Vollan]

Created attachment 335710 [details]
Patch

[Per Arne Vollan]

Created attachment 335713 [details]
Patch

[Per Arne Vollan]

This patch fixes the test compositing/animation/animation-backing.html, which fails when the WebContent process does not have WindowServer access.

[Per Arne Vollan]

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.

[Per Arne Vollan]

Created attachment 335720 [details]
Patch

[Per Arne Vollan]

Created attachment 335734 [details]
Patch

[Per Arne Vollan]

The test animations/animation-callback-timestamp.html, which is measuring the performance of requestAnimationFrame, is passing with this patch.

[Per Arne Vollan]

Created attachment 335736 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Brent Fulgham]

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.

[Simon Fraser (smfr)]

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

[Per Arne Vollan]

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

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 335887 [details]
Patch

[Per Arne Vollan]

Created attachment 335901 [details]
Patch

[Per Arne Vollan]

Created attachment 335907 [details]
Patch

[Per Arne Vollan]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 335996 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Per Arne Vollan]

Created attachment 336007 [details]
Patch

[Brent Fulgham]

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"

[Per Arne Vollan]

Created attachment 336045 [details]
Patch

[Per Arne Vollan]

Created attachment 336046 [details]
Patch

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 336047 [details]
Patch

[Per Arne Vollan]

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?

[Tim Horton]

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.

[Per Arne Vollan]

(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!

[WebKit Commit Bot]

Comment on attachment 336047 [details]
Patch

Clearing flags on attachment: 336047

Committed r229707: <https://trac.webkit.org/changeset/229707>

[Simon Fraser (smfr)]

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?

[Per Arne Vollan]

(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!

[Tim Horton]

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

[Per Arne Vollan]

(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 :)

[Brent Fulgham]

Comment on attachment 336007 [details]
Patch

Clearing review flag.

[Brent Fulgham]

<rdar://problem/38305109>