WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll is directly acces...
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 Apr 2018 23:34:17 +0000 (23:34 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 9 Apr 2018 23:34:17 +0000 (23:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184344
<rdar://problem/39224969>

Reviewed by Per Arne Vollan.

The implementation of targetPositionInWindowForSelectionAutoscroll uses the display ID to get the
screen boundaries of the current display. This causes a bunch of interaction with NSScreen that
we do not want to allow in the WebContent process.

Instead, we should just use the cached screen information the WebContent process already possesses.

This patch makes the following changes:
1. We now retrieve the screen rect of the page's display from the cache, rather than interacting with
   the WindowServer directly.
2. Add a new 'toUserSpaceForPrimaryScreen' so we don't have to deal with a nil NSWindow when computing
   the user space version of the coordinates. A nil Window just means we want to get coordinates in
   terms of the primary display.
3. Keep track of the primary display so we can refer to it later.
4. Modify the IPC messages to include the primary display's ID so we can easily access it later.
5. Modify the PlatformScreen methods to actually use the primary display when appropriate, rather
   than whichever screen happened to hash to the lowest value.

Source/WebCore:

* page/mac/EventHandlerMac.mm:
(WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll const): Use new methods that
don't require WindowServer access.
* platform/PlatformScreen.h:
* platform/mac/PlatformScreenMac.mm:
(WebCore::displayID): Assert if we hit this code in the WebContent process.
(WebCore::firstScreen): Ditto.
(WebCore::window): Ditto.
(WebCore::screen): Ditto.
(WebCore::primaryScreenID): Added.
(WebCore::getScreenProperties): Modify to return a pair consisting of the primary display ID and
the HashSet of screen settings.
(WebCore::setScreenProperties): Update to also track the primary display ID.
(WebCore::screenProperties): Update to use the primary display ID.
(WebCore::screenHasInvertedColors): Ditto.
(WebCore::toUserSpaceForPrimaryScreen): Added.

Source/WebKit:

Reviewed by Per Arne Vollan.

* UIProcess/WebProcessPool.cpp:
(WebKit::displayReconfigurationCallBack): Update for new getScreenProperties implementation.
(WebKit::WebProcessPool::initializeNewWebProcess): Ditto.
* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::setScreenProperties): Ditto.
* WebProcess/WebProcess.h:
* WebProcess/WebProcess.messages.in: Ditto.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230454 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/page/mac/EventHandlerMac.mm
Source/WebCore/platform/PlatformScreen.h
Source/WebCore/platform/mac/PlatformScreenMac.mm
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebProcessPool.cpp
Source/WebKit/WebProcess/WebProcess.cpp
Source/WebKit/WebProcess/WebProcess.h
Source/WebKit/WebProcess/WebProcess.messages.in

index 5b13541..fe6f588 100644 (file)
@@ -1,3 +1,45 @@
+2018-04-09  Brent Fulgham  <bfulgham@apple.com>
+
+        WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll is directly accessing NSScreen
+        https://bugs.webkit.org/show_bug.cgi?id=184344
+        <rdar://problem/39224969>
+
+        Reviewed by Per Arne Vollan.
+
+        The implementation of targetPositionInWindowForSelectionAutoscroll uses the display ID to get the
+        screen boundaries of the current display. This causes a bunch of interaction with NSScreen that
+        we do not want to allow in the WebContent process.
+
+        Instead, we should just use the cached screen information the WebContent process already possesses.
+
+        This patch makes the following changes:
+        1. We now retrieve the screen rect of the page's display from the cache, rather than interacting with
+           the WindowServer directly.
+        2. Add a new 'toUserSpaceForPrimaryScreen' so we don't have to deal with a nil NSWindow when computing
+           the user space version of the coordinates. A nil Window just means we want to get coordinates in
+           terms of the primary display.
+        3. Keep track of the primary display so we can refer to it later.
+        4. Modify the IPC messages to include the primary display's ID so we can easily access it later.
+        5. Modify the PlatformScreen methods to actually use the primary display when appropriate, rather
+           than whichever screen happened to hash to the lowest value.
+
+        * page/mac/EventHandlerMac.mm:
+        (WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll const): Use new methods that
+        don't require WindowServer access.
+        * platform/PlatformScreen.h:
+        * platform/mac/PlatformScreenMac.mm:
+        (WebCore::displayID): Assert if we hit this code in the WebContent process.
+        (WebCore::firstScreen): Ditto.
+        (WebCore::window): Ditto.
+        (WebCore::screen): Ditto.
+        (WebCore::primaryScreenID): Added.
+        (WebCore::getScreenProperties): Modify to return a pair consisting of the primary display ID and
+        the HashSet of screen settings.
+        (WebCore::setScreenProperties): Update to also track the primary display ID.
+        (WebCore::screenProperties): Update to use the primary display ID.
+        (WebCore::screenHasInvertedColors): Ditto.
+        (WebCore::toUserSpaceForPrimaryScreen): Added.
+
 2018-04-09  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         Make InlineTextBox::createTextRun() take a const lvalue reference String
index ba26e57..f10433e 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2006-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2006-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
 #include "Page.h"
 #include "Pasteboard.h"
 #include "PlatformEventFactoryMac.h"
+#include "PlatformScreen.h"
 #include "Range.h"
 #include "RenderLayer.h"
 #include "RenderListBox.h"
 #include "RenderView.h"
 #include "RenderWidget.h"
 #include "RuntimeApplicationChecks.h"
+#include "ScreenProperties.h"
 #include "ScrollAnimator.h"
 #include "ScrollLatchingState.h"
 #include "ScrollableArea.h"
@@ -1164,7 +1166,7 @@ IntPoint EventHandler::targetPositionInWindowForSelectionAutoscroll() const
     if (!page)
         return m_lastKnownMousePosition;
 
-    auto frame = toUserSpace(screen(page->chrome().displayID()).frame, nil);
+    auto frame = toUserSpaceForPrimaryScreen(screenRectForDisplay(page->chrome().displayID()));
     return m_lastKnownMousePosition + autoscrollAdjustmentFactorForScreenBoundaries(m_lastKnownMouseGlobalPosition, frame);
 }
 
index f54f79a..59100a2 100644 (file)
@@ -84,13 +84,17 @@ struct ScreenProperties;
 NSScreen *screen(NSWindow *);
 NSScreen *screen(PlatformDisplayID);
 
+FloatRect screenRectForDisplay(PlatformDisplayID);
+
 WEBCORE_EXPORT FloatRect toUserSpace(const NSRect&, NSWindow *destination);
+FloatRect toUserSpaceForPrimaryScreen(const NSRect&);
 WEBCORE_EXPORT NSRect toDeviceSpace(const FloatRect&, NSWindow *source);
 
 NSPoint flipScreenPoint(const NSPoint&, NSScreen *);
 
-WEBCORE_EXPORT void getScreenProperties(HashMap<PlatformDisplayID, ScreenProperties>&);
-WEBCORE_EXPORT void setScreenProperties(const HashMap<PlatformDisplayID, ScreenProperties>&);
+WEBCORE_EXPORT std::pair<PlatformDisplayID, HashMap<PlatformDisplayID, ScreenProperties>> getScreenProperties();
+WEBCORE_EXPORT void setScreenProperties(PlatformDisplayID primaryScreenID, const HashMap<PlatformDisplayID, ScreenProperties>&);
+ScreenProperties screenProperties(PlatformDisplayID);
 
 #endif
 
index 27a435d..be8abd8 100644 (file)
@@ -48,7 +48,7 @@ namespace WebCore {
 
 static PlatformDisplayID displayID(NSScreen *screen)
 {
-    // FIXME: <https://webkit.org/b/184344> We should assert here if in WebContent process.
+    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
     return [[[screen deviceDescription] objectForKey:@"NSScreenNumber"] intValue];
 }
 
@@ -71,7 +71,7 @@ static PlatformDisplayID displayID(Widget* widget)
 // Screen containing the menubar.
 static NSScreen *firstScreen()
 {
-    // FIXME: <https://webkit.org/b/184344> We should assert here if in WebContent process.
+    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
     NSArray *screens = [NSScreen screens];
     if (![screens count])
         return nil;
@@ -80,6 +80,7 @@ static NSScreen *firstScreen()
 
 static NSWindow *window(Widget* widget)
 {
+    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
     if (!widget)
         return nil;
     return widget->platformWidget().window;
@@ -87,6 +88,7 @@ static NSWindow *window(Widget* widget)
 
 static NSScreen *screen(Widget* widget)
 {
+    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
     // If the widget is in a window, use that, otherwise use the display ID from the host window.
     // First case is for when the NSWindow is in the same process, second case for when it's not.
     if (auto screenFromWindow = window(widget).screen)
@@ -100,10 +102,21 @@ static HashMap<PlatformDisplayID, ScreenProperties>& screenProperties()
     return screenProperties;
 }
 
-void getScreenProperties(HashMap<PlatformDisplayID, ScreenProperties>& screenProperties)
+static PlatformDisplayID& primaryScreenDisplayID()
+{
+    static PlatformDisplayID primaryScreenDisplayID = 0;
+    return primaryScreenDisplayID;
+}
+
+std::pair<PlatformDisplayID, HashMap<PlatformDisplayID, ScreenProperties>> getScreenProperties()
 {
     RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
+
+    HashMap<PlatformDisplayID, ScreenProperties> screenProperties;
+    std::optional<PlatformDisplayID> firstScreen;
+
     for (NSScreen *screen in [NSScreen screens]) {
+        auto displayID = WebCore::displayID(screen);
         FloatRect screenAvailableRect = screen.visibleFrame;
         screenAvailableRect.setY(NSMaxY(screen.frame) - (screenAvailableRect.y() + screenAvailableRect.height())); // flip
         FloatRect screenRect = screen.frame;
@@ -116,28 +129,42 @@ void getScreenProperties(HashMap<PlatformDisplayID, ScreenProperties>& screenPro
         bool screenHasInvertedColors = CGDisplayUsesInvertedPolarity();
         bool screenIsMonochrome = CGDisplayUsesForceToGray();
 
-        screenProperties.set(WebCore::displayID(screen), ScreenProperties { screenAvailableRect, screenRect, colorSpace, screenDepth, screenDepthPerComponent, screenSupportsExtendedColor, screenHasInvertedColors, screenIsMonochrome });
+        screenProperties.set(displayID, ScreenProperties { screenAvailableRect, screenRect, colorSpace, screenDepth, screenDepthPerComponent, screenSupportsExtendedColor, screenHasInvertedColors, screenIsMonochrome });
+
+        if (!firstScreen)
+            firstScreen = displayID;
     }
+
+    return { WTFMove(*firstScreen), WTFMove(screenProperties) };
 }
 
-void setScreenProperties(const HashMap<PlatformDisplayID, ScreenProperties>& properties)
+void setScreenProperties(PlatformDisplayID primaryScreenID, const HashMap<PlatformDisplayID, ScreenProperties>& properties)
 {
+    primaryScreenDisplayID() = primaryScreenID;
     screenProperties() = properties;
 }
-    
-static ScreenProperties getScreenProperties(Widget* widget)
+
+ScreenProperties screenProperties(PlatformDisplayID screendisplayID)
 {
-    auto displayIDForWidget = displayID(widget);
-    if (displayIDForWidget) {
-        auto screenPropertiesForDisplay = screenProperties().find(displayIDForWidget);
+    RELEASE_ASSERT(!screenProperties().isEmpty());
+
+    // Return property of the first screen if the screen is not found in the map.
+    auto displayID = screendisplayID ? screendisplayID : primaryScreenDisplayID();
+    if (displayID) {
+        auto screenPropertiesForDisplay = screenProperties().find(displayID);
         if (screenPropertiesForDisplay != screenProperties().end())
             return screenPropertiesForDisplay->value;
     }
 
-    // Return property of the first screen if the screen is not found in the map.
+    // Last resort: use the first item in the screen list.
     return screenProperties().begin()->value;
 }
 
+static ScreenProperties getScreenProperties(Widget* widget)
+{
+    return screenProperties(displayID(widget));
+}
+
 bool screenIsMonochrome(Widget* widget)
 {
     if (!screenProperties().isEmpty())
@@ -151,7 +178,7 @@ bool screenIsMonochrome(Widget* widget)
 bool screenHasInvertedColors()
 {
     if (!screenProperties().isEmpty())
-        return screenProperties().begin()->value.screenHasInvertedColors;
+        return screenProperties(primaryScreenDisplayID()).screenHasInvertedColors;
 
     // This is a system-wide accessibility setting, same on all screens.
     RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
@@ -182,6 +209,18 @@ int screenDepthPerComponent(Widget* widget)
     return NSBitsPerSampleFromDepth(screen(widget).depth);
 }
 
+FloatRect screenRectForDisplay(PlatformDisplayID displayID)
+{
+    if (!screenProperties().isEmpty()) {
+        auto screenRect = screenProperties(displayID).screenRect;
+        ASSERT(!screenRect.isEmpty());
+        return screenRect;
+    }
+
+    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
+    return screen(displayID).frame;
+}
+
 FloatRect screenRect(Widget* widget)
 {
     if (!screenProperties().isEmpty())
@@ -202,13 +241,13 @@ FloatRect screenAvailableRect(Widget* widget)
 
 NSScreen *screen(NSWindow *window)
 {
-    // FIXME: <https://webkit.org/b/184344> We should assert here if in WebContent process.
+    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
     return [window screen] ?: firstScreen();
 }
 
 NSScreen *screen(PlatformDisplayID displayID)
 {
-    // FIXME: <https://webkit.org/b/184344> We should assert here if in WebContent process.
+    RELEASE_ASSERT(hasProcessPrivilege(ProcessPrivilege::CanCommunicateWithWindowServer));
     for (NSScreen *screen in [NSScreen screens]) {
         if (WebCore::displayID(screen) == displayID)
             return screen;
@@ -244,6 +283,13 @@ FloatRect toUserSpace(const NSRect& rect, NSWindow *destination)
     return userRect;
 }
 
+FloatRect toUserSpaceForPrimaryScreen(const NSRect& rect)
+{
+    FloatRect userRect = rect;
+    userRect.setY(NSMaxY(screenRectForDisplay(primaryScreenDisplayID())) - (userRect.y() + userRect.height())); // flip
+    return userRect;
+}
+
 NSRect toDeviceSpace(const FloatRect& rect, NSWindow *source)
 {
     FloatRect deviceRect = rect;
index fd02d98..82945de 100644 (file)
@@ -1,3 +1,38 @@
+2018-04-09  Brent Fulgham  <bfulgham@apple.com>
+
+        WebCore::EventHandler::targetPositionInWindowForSelectionAutoscroll is directly accessing NSScreen
+        https://bugs.webkit.org/show_bug.cgi?id=184344
+        <rdar://problem/39224969>
+
+        Reviewed by Per Arne Vollan.
+
+        The implementation of targetPositionInWindowForSelectionAutoscroll uses the display ID to get the
+        screen boundaries of the current display. This causes a bunch of interaction with NSScreen that
+        we do not want to allow in the WebContent process.
+
+        Instead, we should just use the cached screen information the WebContent process already possesses.
+
+        This patch makes the following changes:
+        1. We now retrieve the screen rect of the page's display from the cache, rather than interacting with
+           the WindowServer directly.
+        2. Add a new 'toUserSpaceForPrimaryScreen' so we don't have to deal with a nil NSWindow when computing
+           the user space version of the coordinates. A nil Window just means we want to get coordinates in
+           terms of the primary display.
+        3. Keep track of the primary display so we can refer to it later.
+        4. Modify the IPC messages to include the primary display's ID so we can easily access it later.
+        5. Modify the PlatformScreen methods to actually use the primary display when appropriate, rather
+           than whichever screen happened to hash to the lowest value.
+
+        Reviewed by Per Arne Vollan.
+
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::displayReconfigurationCallBack): Update for new getScreenProperties implementation.
+        (WebKit::WebProcessPool::initializeNewWebProcess): Ditto.
+        * WebProcess/WebProcess.cpp:
+        (WebKit::WebProcess::setScreenProperties): Ditto.
+        * WebProcess/WebProcess.h:
+        * WebProcess/WebProcess.messages.in: Ditto.
+
 2018-04-09  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         [WPE] Add API version to library soname and pkg-config files
index 70eb7b2..63f7fb0 100644 (file)
@@ -749,11 +749,9 @@ WebProcessProxy& WebProcessPool::createNewWebProcess(WebsiteDataStore& websiteDa
 #if PLATFORM(MAC)
 static void displayReconfigurationCallBack(CGDirectDisplayID display, CGDisplayChangeSummaryFlags flags, void *userInfo)
 {
-    HashMap<PlatformDisplayID, ScreenProperties> screenProperties;
-    WebCore::getScreenProperties(screenProperties);
-
+    auto screenProperties = WebCore::getScreenProperties();
     for (auto& processPool : WebProcessPool::allProcessPools())
-        processPool->sendToAllProcesses(Messages::WebProcess::SetScreenProperties(screenProperties));
+        processPool->sendToAllProcesses(Messages::WebProcess::SetScreenProperties(screenProperties.first, screenProperties.second));
 }
 
 static void registerDisplayConfigurationCallback()
@@ -918,9 +916,8 @@ void WebProcessPool::initializeNewWebProcess(WebProcessProxy& process, WebsiteDa
 #if PLATFORM(MAC)
     registerDisplayConfigurationCallback();
 
-    HashMap<PlatformDisplayID, ScreenProperties> screenProperties;
-    WebCore::getScreenProperties(screenProperties);
-    process.send(Messages::WebProcess::SetScreenProperties(screenProperties), 0);
+    auto screenProperties = WebCore::getScreenProperties();
+    process.send(Messages::WebProcess::SetScreenProperties(screenProperties.first, screenProperties.second), 0);
 #endif
 }
 
index 4ec594b..4ccc5e1 100644 (file)
@@ -1699,9 +1699,9 @@ void WebProcess::registerServiceWorkerClients()
 #endif
 
 #if PLATFORM(MAC)
-void WebProcess::setScreenProperties(const HashMap<uint32_t, WebCore::ScreenProperties>& properties)
+void WebProcess::setScreenProperties(uint32_t primaryScreenID, const HashMap<uint32_t, WebCore::ScreenProperties>& properties)
 {
-    WebCore::setScreenProperties(properties);
+    WebCore::setScreenProperties(primaryScreenID, properties);
 }
 #endif
 
index e573936..4d1131a 100644 (file)
@@ -372,7 +372,7 @@ private:
     void didReceiveSyncWebProcessMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>&);
 
 #if PLATFORM(MAC)
-    void setScreenProperties(const HashMap<uint32_t, WebCore::ScreenProperties>&);
+    void setScreenProperties(uint32_t primaryScreenID, const HashMap<uint32_t, WebCore::ScreenProperties>&);
 #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
     void scrollerStylePreferenceChanged(bool useOverlayScrollbars);
 #endif
index 3bd2779..ea695bb 100644 (file)
@@ -129,7 +129,7 @@ messages -> WebProcess LegacyReceiver {
     MessagesAvailableForPort(struct WebCore::MessagePortIdentifier port)
 
 #if PLATFORM(MAC)
-    SetScreenProperties(HashMap<uint32_t, WebCore::ScreenProperties> screenProperties)
+    SetScreenProperties(uint32_t primaryScreenID, HashMap<uint32_t, WebCore::ScreenProperties> screenProperties)
 #if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101400
     ScrollerStylePreferenceChanged(bool useOvelayScrollbars)
 #endif