Clean up Win32Handle implementation #13611

donny-dont · 2023-05-08T23:58:39Z

`5841938`

Clean up Win32Handle implementation
https://bugs.webkit.org/show_bug.cgi?id=256498

Reviewed by Fujii Hironori.

Make `Win32Handle` look like `MachSendPort` so it has a similar
interface.

* Source/WTF/wtf/PlatformWin.cmake:
* Source/WTF/wtf/win/MemoryFootprintWin.cpp:
* Source/WTF/wtf/win/MemoryPressureHandlerWin.cpp:
* Source/WTF/wtf/win/Win32Handle.cpp: Added.
* Source/WTF/wtf/win/Win32Handle.h:
* Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp:
* Source/WebKit/Platform/IPC/Connection.h:
* Source/WebKit/Platform/IPC/win/ArgumentCodersWin.cpp:
* Source/WebKit/Platform/IPC/win/ConnectionWin.cpp:
* Source/WebKit/Platform/IPC/win/IPCSemaphoreWin.cpp:
* Source/WebKit/Platform/win/SharedMemoryWin.cpp:
* Source/WebKit/UIProcess/Launcher/win/ProcessLauncherWin.cpp:

Canonical link: https://commits.webkit.org/263991@main

cef6e5b

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim		✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-05-08T23:58:49Z

EWS run on previous version of this PR (hash a729953)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🧪 api-mac	⏳ 🛠 gtk
	~~🧪 ios-wk2-wpt~~	✅ 🧪 mac-wk1	⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc	~~🧪 api-ios~~	✅ 🧪 mac-wk2	⏳ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	⏳ 🛠 tv-sim		⏳ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	⏳ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-05-09T00:07:56Z

EWS run on previous version of this PR (hash e38e2b0)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc	~~🧪 api-ios~~	✅ 🧪 mac-wk2	⏳ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim		✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-05-09T01:11:56Z

EWS run on previous version of this PR (hash 5ec8245)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	❌ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim		✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

fujii · 2023-05-10T00:08:25Z

I don't know why Win32Handle has to be non-copyable.
std::unique_ptr<T> is non-copyable because its ownership is unique.
std::shared_ptr<T> isn't non-copyable because it can copy.
Win32Handle is actually copyable. So, you can have Win32Handle::duplicate().

donny-dont · 2023-05-10T00:13:49Z

Its more about explicit ownership and being very explicit when you're duplicating a handle which follows along with what is happening in the UnixFileDescriptor implementation.

Source/WTF/wtf/win/MemoryFootprintWin.cpp

webkit-early-warning-system · 2023-05-10T17:10:45Z

EWS run on previous version of this PR (hash cdc45d2)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim		✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

kkinnunen-apple · 2023-05-10T19:25:17Z

Source/WTF/wtf/win/Win32Handle.h

I think all handle like objects should be eventually have the same interface:
Win32Handle, MachSendRight, UnixFileDescriptor.

For adoption tag: I'd prefer the unfication happens in other direction, e.g. removal of the public adoption constructor and tag.

All webkit types seem to use specific adoption constructor function:

MachSendRight::adopt

adoptRef(T*)

others

This means that there is no other precedent to use adopt tag except UnixFileDescriptor.

Instead, I'd propose we have:

Win32Handle::adopt

UnixFileDescriptor::adopt

MachSendRight::adopt

webkit-early-warning-system · 2023-05-11T20:16:15Z

EWS run on previous version of this PR (hash e0a1ca3)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	~~🧪 gtk-wk2~~
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	~~🧪 mac-wk2~~	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim		✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

fujii · 2023-05-11T20:34:09Z

Source/WTF/wtf/win/Win32Handle.h

This isn't needed. Please remove it.

fujii · 2023-05-11T20:34:18Z

Source/WTF/wtf/win/Win32Handle.h

This isn't needed. Please remove it.

fujii · 2023-05-11T20:34:33Z

Source/WTF/wtf/win/Win32Handle.h

This isn't needed. Please remove it.

fujii · 2023-05-11T20:35:20Z

Source/WTF/wtf/win/Win32Handle.cpp

This isn't needed. Please remove it. Move this code to copy().

webkit-early-warning-system · 2023-05-11T22:01:29Z

EWS run on previous version of this PR (hash fdfe305)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	✅ 🛠 gtk
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	⏳ 🧪 gtk-wk2
⏳ 🛠 🧪 jsc	~~🧪 api-ios~~	~~🧪 mac-wk2~~	~~🧪 api-gtk~~
⏳ 🛠 🧪 jsc-arm64	✅ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim		✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

webkit-early-warning-system · 2023-05-11T22:12:06Z

EWS run on current version of this PR (hash cef6e5b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🛠 gtk
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	✅ 🛠 jsc-armv7
	✅ 🛠 tv-sim		✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 merge	✅ 🛠 watch		✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

https://bugs.webkit.org/show_bug.cgi?id=256498 Reviewed by Fujii Hironori. Make `Win32Handle` look like `MachSendPort` so it has a similar interface. * Source/WTF/wtf/PlatformWin.cmake: * Source/WTF/wtf/win/MemoryFootprintWin.cpp: * Source/WTF/wtf/win/MemoryPressureHandlerWin.cpp: * Source/WTF/wtf/win/Win32Handle.cpp: Added. * Source/WTF/wtf/win/Win32Handle.h: * Source/WebCore/platform/graphics/win/MediaPlayerPrivateMediaFoundation.cpp: * Source/WebKit/Platform/IPC/Connection.h: * Source/WebKit/Platform/IPC/win/ArgumentCodersWin.cpp: * Source/WebKit/Platform/IPC/win/ConnectionWin.cpp: * Source/WebKit/Platform/IPC/win/IPCSemaphoreWin.cpp: * Source/WebKit/Platform/win/SharedMemoryWin.cpp: * Source/WebKit/UIProcess/Launcher/win/ProcessLauncherWin.cpp: Canonical link: https://commits.webkit.org/263991@main

webkit-commit-queue · 2023-05-11T23:55:00Z

Committed 263991@main (5841938): https://commits.webkit.org/263991@main

Reviewed commits have been landed. Closing PR #13611 and removing active labels.

kkinnunen-apple · 2023-05-12T05:35:32Z

Source/WTF/wtf/win/Win32Handle.cpp

+        return INVALID_HANDLE_VALUE;
+
+    auto processHandle = ::GetCurrentProcess();
+    HANDLE duplicate;


If the call fails, you might have uninitialized value here.. The docs don't say..

Good point. Filed https://bugs.webkit.org/show_bug.cgi?id=257435

donny-dont self-assigned this May 8, 2023

donny-dont added the Platform Portability improvements and other general platform improvements not driven directly by site bugs. label May 8, 2023

donny-dont force-pushed the eng/Clean-up-Win32Handle-implementation branch from a729953 to e38e2b0 Compare May 9, 2023 00:07

donny-dont force-pushed the eng/Clean-up-Win32Handle-implementation branch from e38e2b0 to 5ec8245 Compare May 9, 2023 01:10

donny-dont marked this pull request as ready for review May 9, 2023 23:14

donny-dont requested a review from cdumez as a code owner May 9, 2023 23:14

donny-dont requested a review from fujii May 9, 2023 23:14

fujii reviewed May 10, 2023

View reviewed changes

Source/WTF/wtf/win/MemoryFootprintWin.cpp Outdated Show resolved Hide resolved

donny-dont force-pushed the eng/Clean-up-Win32Handle-implementation branch from 5ec8245 to cdc45d2 Compare May 10, 2023 17:09

kkinnunen-apple reviewed May 10, 2023

View reviewed changes

donny-dont force-pushed the eng/Clean-up-Win32Handle-implementation branch from cdc45d2 to e0a1ca3 Compare May 11, 2023 20:15

fujii requested changes May 11, 2023

View reviewed changes

donny-dont force-pushed the eng/Clean-up-Win32Handle-implementation branch from e0a1ca3 to fdfe305 Compare May 11, 2023 22:01

fujii approved these changes May 11, 2023

View reviewed changes

donny-dont force-pushed the eng/Clean-up-Win32Handle-implementation branch from fdfe305 to cef6e5b Compare May 11, 2023 22:10

donny-dont added the merge-queue Applied to send a pull request to merge-queue label May 11, 2023

webkit-commit-queue force-pushed the eng/Clean-up-Win32Handle-implementation branch from cef6e5b to 5841938 Compare May 11, 2023 23:54

webkit-commit-queue merged commit 5841938 into WebKit:main May 11, 2023

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 11, 2023

kkinnunen-apple reviewed May 12, 2023

View reviewed changes

Clean up Win32Handle implementation #13611

Clean up Win32Handle implementation #13611

Uh oh!

Conversation

donny-dont commented May 8, 2023 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

5841938

Uh oh!

webkit-early-warning-system commented May 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fujii commented May 10, 2023

Uh oh!

donny-dont commented May 10, 2023

Uh oh!

Uh oh!

webkit-early-warning-system commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kkinnunen-apple May 10, 2023

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fujii May 11, 2023

Choose a reason for hiding this comment

Uh oh!

fujii May 11, 2023

Choose a reason for hiding this comment

Uh oh!

fujii May 11, 2023

Choose a reason for hiding this comment

Uh oh!

fujii May 11, 2023

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented May 11, 2023

Uh oh!

kkinnunen-apple May 12, 2023

Choose a reason for hiding this comment

Uh oh!

fujii May 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

donny-dont commented May 8, 2023 •

edited by webkit-early-warning-system

Loading

`5841938`

webkit-early-warning-system commented May 8, 2023 •

edited

Loading

webkit-early-warning-system commented May 9, 2023 •

edited

Loading

webkit-early-warning-system commented May 9, 2023 •

edited

Loading

webkit-early-warning-system commented May 10, 2023 •

edited

Loading

webkit-early-warning-system commented May 11, 2023 •

edited

Loading

webkit-early-warning-system commented May 11, 2023 •

edited

Loading

webkit-early-warning-system commented May 11, 2023 •

edited

Loading

fujii May 28, 2023 •

edited

Loading