Summary: | The WebContent process should not use NSScreen in the screenAvailableRect/screenRect implementations. | ||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||||||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, beidson, bfulgham, cdumez, commit-queue, darin, dbates, ews-watchlist, japhet, rniwa, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 182747, 183338 | ||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Per Arne Vollan
2018-02-15 18:09:35 PST
Created attachment 333979 [details]
Patch
Created attachment 333984 [details]
Patch
Created attachment 333987 [details]
Patch
Created attachment 333989 [details]
Patch
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? (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! 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 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
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 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
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 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
Created attachment 333997 [details]
Patch
I will attempt to do this with async IPC. Created attachment 334176 [details]
Patch
Created attachment 334178 [details]
Patch
Created attachment 334181 [details]
Patch
(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'. (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. Created attachment 334184 [details]
Patch
(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? 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. Created attachment 334278 [details]
Patch
(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! Created attachment 334283 [details]
Patch
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. 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); (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. Created attachment 334290 [details]
Patch
Created attachment 334293 [details]
Patch
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 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
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? Created attachment 334399 [details]
Patch
(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! Created attachment 334403 [details]
Patch
Created attachment 334407 [details]
Patch
Created attachment 334414 [details]
Patch
Comment on attachment 334414 [details]
Patch
r=me
Comment on attachment 334414 [details]
Patch
Thanks for reviewing!
Comment on attachment 334414 [details] Patch Clearing flags on attachment: 334414 Committed r228907: <https://trac.webkit.org/changeset/228907> All reviewed patches have been landed. Closing bug. 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. 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. (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. 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. (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. (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. (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. |