Bug 182855 – The WebContent process should not use NSScreen in the screenAvailableRect/screenRect implementations.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

The WebContent process should broker NSScreen method calls to the UIProcess, since these calls will communicate with the WindowServer.

[Per Arne Vollan]

Created attachment 333979 [details]
Patch

[Per Arne Vollan]

Created attachment 333984 [details]
Patch

[Per Arne Vollan]

Created attachment 333987 [details]
Patch

[Per Arne Vollan]

Created attachment 333989 [details]
Patch

[Chris Dumez]

Comment on attachment 333989 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        since these calls will communicate with the WindowServer.

THis introduces new sync IPC, why do we want this? Do we really need sync IPC?

[Per Arne Vollan]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 333989 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333989&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        since these calls will communicate with the WindowServer.
> 
> THis introduces new sync IPC, why do we want this? Do we really need sync
> IPC?

I also think it would be better with async IPC. I will look into whether we can get the screen size asynchronously.

Thanks for reviewing!

[EWS Watchlist]

Comment on attachment 333989 [details]
Patch

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

New failing tests:
fast/frames/iframe-access-screen-of-deleted.html
fast/dom/Window/property-access-on-cached-window-after-frame-removed.html

[EWS Watchlist]

Created attachment 333992 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 333989 [details]
Patch

Attachment 333989 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6528759

New failing tests:
fast/frames/iframe-access-screen-of-deleted.html
fast/dom/Window/property-access-on-cached-window-after-frame-removed.html

[EWS Watchlist]

Created attachment 333995 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 333989 [details]
Patch

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

New failing tests:
fast/frames/iframe-access-screen-of-deleted.html
fast/dom/Window/property-access-on-cached-window-after-frame-removed.html

[EWS Watchlist]

Created attachment 333996 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Per Arne Vollan]

Created attachment 333997 [details]
Patch

[Per Arne Vollan]

I will attempt to do this with async IPC.

[Per Arne Vollan]

Created attachment 334176 [details]
Patch

[Per Arne Vollan]

Created attachment 334178 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/37683058>

[Per Arne Vollan]

Created attachment 334181 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #14)
> I will attempt to do this with async IPC.

It seems we might not be able to do this with async IPC, since the function is used when e.g. getting the DOM property 'window.screen.availWidth'.

[Chris Dumez]

(In reply to Per Arne Vollan from comment #19)
> (In reply to Per Arne Vollan from comment #14)
> > I will attempt to do this with async IPC.
> 
> It seems we might not be able to do this with async IPC, since the function
> is used when e.g. getting the DOM property 'window.screen.availWidth'.

Once way would be to cache those on the WebContent process side and have the UIProcess notify the WebProcesses whenever these values change.

[Per Arne Vollan]

Created attachment 334184 [details]
Patch

[Per Arne Vollan]

(In reply to Chris Dumez from comment #20)
> (In reply to Per Arne Vollan from comment #19)
> > (In reply to Per Arne Vollan from comment #14)
> > > I will attempt to do this with async IPC.
> > 
> > It seems we might not be able to do this with async IPC, since the function
> > is used when e.g. getting the DOM property 'window.screen.availWidth'.
> 
> Once way would be to cache those on the WebContent process side and have the
> UIProcess notify the WebProcesses whenever these values change.

Right, that is a good idea. Do we get any notifications from the system when these display properties change?

[Alex Christensen]

Comment on attachment 334184 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.messages.in:519
> +    ScreenAvailableRect() -> (WebCore::FloatRect screen) Delayed

This is still synchronous IPC.

[Per Arne Vollan]

Created attachment 334278 [details]
Patch

[Per Arne Vollan]

(In reply to Alex Christensen from comment #23)
> Comment on attachment 334184 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334184&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.messages.in:519
> > +    ScreenAvailableRect() -> (WebCore::FloatRect screen) Delayed
> 
> This is still synchronous IPC.

I have now implemented Chris' proposal in the latest patch, which should send messages async.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 334283 [details]
Patch

[Brent Fulgham]

Comment on attachment 334283 [details]
Patch

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

> Source/WebCore/platform/mac/PlatformScreenMac.mm:120
> +void getScreenAvailableRects(HashMap<PlatformDisplayID, FloatRect>& rects)

(Not for this bug) All of this should probably be in the "Platform Abstraction Layer" library.

[Brent Fulgham]

Comment on attachment 334283 [details]
Patch

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

r=me

> Source/WebCore/platform/mac/PlatformScreenMac.mm:142
> +    if (screenRects().contains(WebCore::displayID(screen(widget)))) {

Is the result of WebCore::displayID(screen(widget)) worth capturing in a variable? I'm not sure how costly it is to do this.

> Source/WebCore/platform/mac/PlatformScreenMac.mm:144
> +        return rect;

... then this could just be:

return screenRects().get(displayIDForWidget);

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #28)
> Comment on attachment 334283 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334283&action=review
> 
> r=me
> 
> > Source/WebCore/platform/mac/PlatformScreenMac.mm:142
> > +    if (screenRects().contains(WebCore::displayID(screen(widget)))) {
> 
> Is the result of WebCore::displayID(screen(widget)) worth capturing in a
> variable? I'm not sure how costly it is to do this.
> 
> > Source/WebCore/platform/mac/PlatformScreenMac.mm:144
> > +        return rect;
> 
> ... then this could just be:
> 
> return screenRects().get(displayIDForWidget);

Thanks for reviewing! I will update the patch before landing.

[Per Arne Vollan]

Created attachment 334290 [details]
Patch

[Per Arne Vollan]

Created attachment 334293 [details]
Patch

[EWS Watchlist]

Comment on attachment 334283 [details]
Patch

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

New failing tests:
http/tests/preload/onerror_event.html

[EWS Watchlist]

Created attachment 334295 [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

[Daniel Bates]

Does this patch cover the case where a person attaches a new monitor and hen moves the window to the new monitor? Does it handle the case where a window is moved to another monitor if your disconnect the monitor the monitor it was originally on?

[Per Arne Vollan]

Created attachment 334399 [details]
Patch

[Per Arne Vollan]

(In reply to Daniel Bates from comment #34)
> Does this patch cover the case where a person attaches a new monitor and hen
> moves the window to the new monitor? Does it handle the case where a window
> is moved to another monitor if your disconnect the monitor the monitor it
> was originally on?

In theory, the behavior should not change, since we still will use the PlatformDisplayID sent from the UI process to find the properties of the screen on the WebProcess side, and the UI process should send new screen properties to the WebProcess on every screen configuration change.

However, I think I will need to test this manually with the new patch.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 334403 [details]
Patch

[Per Arne Vollan]

Created attachment 334407 [details]
Patch

[Per Arne Vollan]

Created attachment 334414 [details]
Patch

[Brent Fulgham]

Comment on attachment 334414 [details]
Patch

r=me

[Per Arne Vollan]

Comment on attachment 334414 [details]
Patch

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 334414 [details]
Patch

Clearing flags on attachment: 334414

Committed r228907: <https://trac.webkit.org/changeset/228907>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

This change is a good idea for better performance, too, making it possible to eliminate some synchronous messaging from web process to the UI process. If I remember analysis I did earlier correctly, this should boost the speed of Speedometer.

[Darin Adler]

Comment on attachment 334414 [details]
Patch

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

Some ideas for coding style refinement.

> Source/WebCore/platform/PlatformScreen.h:92
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400

What here is specific to newer versions of macOS?

> Source/WebCore/platform/PlatformScreen.h:94
> +WEBCORE_EXPORT void getScreenProperties(HashMap<PlatformDisplayID, ScreenProperties>&);
> +WEBCORE_EXPORT void setScreenProperties(const HashMap<PlatformDisplayID, ScreenProperties>&);

I don’t understand why this can only be done on newer versions of macOS.

The getScreenProperties call should use a return value instead of an out argument. There’s no reason not to do that with C++ with modern move semantics.

The setScreenProperties call should take a HashMap&&, because given the way it’s used, we can move the HashMap in, instead of copying it.

These functions are named peculiarly; the two aren’t parallel! The first one computes the properties and would be inefficient to use over and over again; it’s specifically designed to be called in the UI process only. The second one sets cached properties that will be used internally by all the other functions in this header. I think they might need different names.

> Source/WebCore/platform/mac/PlatformScreenMac.mm:142
> +    if (!screenProperties().isEmpty()) {

I don’t think we should be calling through to NSScreen if the screen properties map just happens to be empty because we are early in the initialization process. We could do something else like:

1) returning a fixed rectangle
2) returning an empty rectangle
3) asserting this case never occurs

I think maybe a combination of (1) and (3) is the best policy.

> Source/WebCore/platform/mac/PlatformScreenMac.mm:145
> +        if (displayIDForWidget && screenProperties().contains(displayIDForWidget))
> +            return screenProperties().get(displayIDForWidget).screenRect;

Double hashing here isn’t good. Should use find instead of contains/get.

> Source/WebCore/platform/mac/PlatformScreenMac.mm:146
> +        // Return property of the first screen if the screen is not found in the map.

This comment says that the code will return the property of the "first screen", which is a good idea, but the code below doesn’t actually do that! It returns the property of a random screen. If we want to know which is the first screen, we will need to change the data structure to identify which is the first; a HashMap alone won’t do that. The rule implemented below is based on the first value seen when iterating the hash map, which will be random based on the hash function, and the current state of the hash table, including things like the current number of buckets.

> Source/WebCore/platform/mac/PlatformScreenMac.mm:148
> +        auto iter = screenProperties().begin();
> +        return screenProperties().get(iter->key).screenRect;

This does an unnecessary hash lookup. The efficient way to write it is one of these:

    return screenProperties().begin()->value.screenRect;

    return screenProperties().values().begin()->screenRect;

But those are only good if we want a randomly chosen screen’s result.

> Source/WebCore/platform/mac/PlatformScreenMac.mm:164
> +    if (!screenProperties().isEmpty()) {
> +        auto displayIDForWidget = displayID(widget);
> +        if (displayIDForWidget && screenProperties().contains(displayIDForWidget))
> +            return screenProperties().get(displayIDForWidget).screenAvailableRect;
> +        // Return property of the first screen if the screen is not found in the map.
> +        auto iter = screenProperties().begin();
> +        return screenProperties().get(iter->key).screenAvailableRect;
> +    }

This function and the one above are nearly identical. Should factor them so they can share code. We can make a helper function that returns a const ScreenProperties* or const ScreenProperties& given a Widget*.

> Source/WebKit/UIProcess/WebProcessPool.cpp:891
> +    HashMap<PlatformDisplayID, ScreenProperties> screenProperties;
> +    WebCore::getScreenProperties(screenProperties);
> +    process.send(Messages::WebProcess::SetScreenProperties(screenProperties), 0);

We should consider passing these in as part of the arguments to InitializeWebProcess instead of sending them as a second messages.

[Darin Adler]

(In reply to Darin Adler from comment #44)
> This change is a good idea for better performance, too, making it possible
> to eliminate some synchronous messaging from web process to the UI process.
> If I remember analysis I did earlier correctly, this should boost the speed
> of Speedometer.

I guess maybe I was remembering something old. I guess we got rid of the IPC a while back and made the web process do NSScreen calls. But this is a better solution.

[Darin Adler]

One more thought: Ideally, I think that the idea that the UI process should compute the rectangles and send them to the web process is something we should implement cross-platform, not just for Mac. The platform-specific part should be how we compute the rectangles. Might require a bit more refactoring to get rid of some of the legacy concepts inside the web process.

[Chris Dumez]

(In reply to Darin Adler from comment #46)
> (In reply to Darin Adler from comment #44)
> > This change is a good idea for better performance, too, making it possible
> > to eliminate some synchronous messaging from web process to the UI process.
> > If I remember analysis I did earlier correctly, this should boost the speed
> > of Speedometer.
> 
> I guess maybe I was remembering something old. I guess we got rid of the IPC
> a while back and made the web process do NSScreen calls. But this is a
> better solution.

I seem to remember we fixed that by not computing the screen coordinates for simulated mouse events. We did not introduce NSScreen calls.

[Per Arne Vollan]

(In reply to Darin Adler from comment #47)
> One more thought: Ideally, I think that the idea that the UI process should
> compute the rectangles and send them to the web process is something we
> should implement cross-platform, not just for Mac. The platform-specific
> part should be how we compute the rectangles. Might require a bit more
> refactoring to get rid of some of the legacy concepts inside the web process.

Thanks for reviewing, Darin! I will create a follow-up patch addressing these issues.

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #49)
> (In reply to Darin Adler from comment #47)
> > One more thought: Ideally, I think that the idea that the UI process should
> > compute the rectangles and send them to the web process is something we
> > should implement cross-platform, not just for Mac. The platform-specific
> > part should be how we compute the rectangles. Might require a bit more
> > refactoring to get rid of some of the legacy concepts inside the web process.
> 
> Thanks for reviewing, Darin! I will create a follow-up patch addressing
> these issues.

This is tracked in https://bugs.webkit.org/show_bug.cgi?id=183338.