Minor refactoring of assertion code, v2 #19650

pvollan · 2023-10-27T20:00:15Z

`5d6df46`

Minor refactoring of assertion code, v2
https://bugs.webkit.org/show_bug.cgi?id=263797
rdar://117600174

Reviewed by Brent Fulgham.

This patch contains some minor refactoring of assertions code:

1) Let ProcessAndUIAssertion create method take an AuxiliaryProcessProxy reference, instead of a PID.
2) Let XPCConnectionTerminationWatchdog hold on to a weak AuxiliaryProcessProxy pointer, instead of an XPC connection.
3) Let ProcessThrottler::didConnectToProcess take an AuxiliaryProcessProxy reference, instead of a PID.

* Source/WebKit/Platform/cocoa/XPCUtilities.mm:
(WebKit::terminateWithReason):
* Source/WebKit/Platform/spi/Cocoa/ExtensionKitSPI.h:
* Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:
(WebKit::AuxiliaryProcessProxy::didFinishLaunching):
* Source/WebKit/UIProcess/Cocoa/AuxiliaryProcessProxyCocoa.mm:
(WebKit::AuxiliaryProcessProxy::platformStartConnectionTerminationWatchdog):
(WebKit::AuxiliaryProcessProxy::extensionProcess const):
(WebKit::AuxiliaryProcessProxy::extensionProcess): Deleted.
* Source/WebKit/UIProcess/Cocoa/ProcessAssertionCocoa.mm:
(WebKit::ProcessAndUIAssertion::ProcessAndUIAssertion):
* Source/WebKit/UIProcess/Cocoa/XPCConnectionTerminationWatchdog.h:
* Source/WebKit/UIProcess/Cocoa/XPCConnectionTerminationWatchdog.mm:
(WebKit::XPCConnectionTerminationWatchdog::startConnectionTerminationWatchdog):
(WebKit::XPCConnectionTerminationWatchdog::XPCConnectionTerminationWatchdog):
(WebKit::XPCConnectionTerminationWatchdog::watchdogTimerFired):
* Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp:
(WebKit::GPUProcessProxy::didFinishLaunching):
* Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:
(WebKit::NetworkProcessProxy::didFinishLaunching):
* Source/WebKit/UIProcess/ProcessAssertion.h:
* Source/WebKit/UIProcess/ProcessThrottler.cpp:
(WebKit::ProcessThrottler::setThrottleState):
(WebKit::ProcessThrottler::updateThrottleStateIfNeeded):
(WebKit::ProcessThrottler::didConnectToProcess):
(WebKit::ProcessThrottler::didDisconnectFromProcess):
(WebKit::ProcessThrottler::sendPrepareToSuspendIPC):
(WebKit::ProcessThrottler::setShouldTakeNearSuspendedAssertion):
(WebKit::ProcessThrottler::isSuspended const):
(WebKit::ProcessThrottlerActivity::ProcessThrottlerActivity):
(WebKit::ProcessThrottlerActivity::invalidate):
* Source/WebKit/UIProcess/ProcessThrottler.h:
(WebKit::ProcessThrottler::isSuspended const): Deleted.
(WebKit::ProcessThrottlerActivity::ProcessThrottlerActivity): Deleted.
(WebKit::ProcessThrottlerActivity::invalidate): Deleted.
* Source/WebKit/UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::updateAudibleMediaAssertions):
* Source/WebKit/UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::didFinishLaunching):
(WebKit::WebProcessProxy::updateAudibleMediaAssertions):

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

6f8231b

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

webkit-early-warning-system · 2023-10-27T20:00:28Z

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

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

webkit-early-warning-system · 2023-11-01T22:00:10Z

EWS run on previous version of this PR (hash 6bc0f8e)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	⏳ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 wincairo
	⏳ 🛠 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
	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2-stress
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-11-01T22:03:47Z

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

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	⏳ 🛠 mac	❌ 🛠 wpe	⏳ 🛠 wincairo
	✅ 🛠 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
	⏳ 🛠 tv-sim	⏳ 🧪 mac-wk2-stress
	⏳ 🛠 watch
	⏳ 🛠 watch-sim

webkit-early-warning-system · 2023-11-01T23:01:44Z

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

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

brentfulgham

I think this looks fine, but I strongly recommend you restore the PID in the logging, as I've frequently needed this information to diagnose problem on users' devices from logs.

brentfulgham · 2023-11-01T22:42:30Z

Source/WebKit/UIProcess/ProcessAssertion.h

@@ -44,7 +44,7 @@ OBJC_CLASS WKRBSAssertionDelegate;

 #if USE(EXTENSIONKIT_ASSERTIONS)
 OBJC_CLASS _SEExtensionProcess;
-OBJC_CLASS _SEGrant;
+OBJC_PROTOCOL(_SEGrant);


Good catch!

brentfulgham · 2023-11-01T22:44:31Z

Source/WebKit/UIProcess/ProcessAssertion.h

 #endif
+    static Ref<ProcessAssertion> create(ProcessID, const String& reason, ProcessAssertionType, Mode = Mode::Async, const String& environmentIdentifier = emptyString(), CompletionHandler<void()>&& acquisisionHandler = nullptr);


nit: Noticed an old typo here: acquisisionHandler -> aquisitionHandler

brentfulgham · 2023-11-01T23:51:33Z

Source/WebKit/UIProcess/Cocoa/XPCConnectionTerminationWatchdog.mm

 {
    m_watchdogTimer.startOneShot(interval);
 }

 void XPCConnectionTerminationWatchdog::watchdogTimerFired()
 {
-    terminateWithReason(m_xpcConnection.get(), ReasonCode::WatchdogTimerFired, "XPCConnectionTerminationWatchdog::watchdogTimerFired");
+    if (m_process && m_process->connection())


If m_process->connection() exists, is the result of xpcConnection() guaranteed to be a valid pointer? If so, why isn't it returning a reference?

It looks like methods such as Connection::getAuditToken() check this value for nullptr. I think we should check connection's validity here before using it to terminate, unless the terminateWithReason method expects the pointer might be nullptr.

It looks like the implementation of xpc_connection_kill uses the passed in argument without nullptr checking, so this would create crashes in some cases.

Ah, good catch! I added a null check before calling xpc_connection_kill.

Thanks for reviewing!

brentfulgham · 2023-11-02T00:21:09Z

Source/WebKit/UIProcess/ProcessThrottler.cpp


 void ProcessThrottler::didDisconnectFromProcess()
 {
-    PROCESSTHROTTLER_RELEASE_LOG_WITH_PID("didDisconnectFromProcess:", m_processID);
+    PROCESSTHROTTLER_RELEASE_LOG("didDisconnectFromProcess:");


I hate the idea of losing this logging. Is there a way to determine the PID from the m_processProxy so we can keep this useful diagnostic information?

Maybe:
PROCESSTHROTTLER_RELEASE_LOG_WITH_PID("didDisconnectFromProcess:", m_processProxy ? m_processProxy->processID() : 0);

The macro PROCESSTHROTTLER_RELEASE_LOG actually logs the PID, so I believe we are good here.

webkit-early-warning-system · 2023-11-02T14:25:29Z

EWS run on previous version of this PR (hash 90fb10a)

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

webkit-early-warning-system · 2023-11-03T19:09:47Z

EWS run on current version of this PR (hash 6f8231b)

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

https://bugs.webkit.org/show_bug.cgi?id=263797 rdar://117600174 Reviewed by Brent Fulgham. This patch contains some minor refactoring of assertions code: 1) Let ProcessAndUIAssertion create method take an AuxiliaryProcessProxy reference, instead of a PID. 2) Let XPCConnectionTerminationWatchdog hold on to a weak AuxiliaryProcessProxy pointer, instead of an XPC connection. 3) Let ProcessThrottler::didConnectToProcess take an AuxiliaryProcessProxy reference, instead of a PID. * Source/WebKit/Platform/cocoa/XPCUtilities.mm: (WebKit::terminateWithReason): * Source/WebKit/Platform/spi/Cocoa/ExtensionKitSPI.h: * Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp: (WebKit::AuxiliaryProcessProxy::didFinishLaunching): * Source/WebKit/UIProcess/Cocoa/AuxiliaryProcessProxyCocoa.mm: (WebKit::AuxiliaryProcessProxy::platformStartConnectionTerminationWatchdog): (WebKit::AuxiliaryProcessProxy::extensionProcess const): (WebKit::AuxiliaryProcessProxy::extensionProcess): Deleted. * Source/WebKit/UIProcess/Cocoa/ProcessAssertionCocoa.mm: (WebKit::ProcessAndUIAssertion::ProcessAndUIAssertion): * Source/WebKit/UIProcess/Cocoa/XPCConnectionTerminationWatchdog.h: * Source/WebKit/UIProcess/Cocoa/XPCConnectionTerminationWatchdog.mm: (WebKit::XPCConnectionTerminationWatchdog::startConnectionTerminationWatchdog): (WebKit::XPCConnectionTerminationWatchdog::XPCConnectionTerminationWatchdog): (WebKit::XPCConnectionTerminationWatchdog::watchdogTimerFired): * Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp: (WebKit::GPUProcessProxy::didFinishLaunching): * Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp: (WebKit::NetworkProcessProxy::didFinishLaunching): * Source/WebKit/UIProcess/ProcessAssertion.h: * Source/WebKit/UIProcess/ProcessThrottler.cpp: (WebKit::ProcessThrottler::setThrottleState): (WebKit::ProcessThrottler::updateThrottleStateIfNeeded): (WebKit::ProcessThrottler::didConnectToProcess): (WebKit::ProcessThrottler::didDisconnectFromProcess): (WebKit::ProcessThrottler::sendPrepareToSuspendIPC): (WebKit::ProcessThrottler::setShouldTakeNearSuspendedAssertion): (WebKit::ProcessThrottler::isSuspended const): (WebKit::ProcessThrottlerActivity::ProcessThrottlerActivity): (WebKit::ProcessThrottlerActivity::invalidate): * Source/WebKit/UIProcess/ProcessThrottler.h: (WebKit::ProcessThrottler::isSuspended const): Deleted. (WebKit::ProcessThrottlerActivity::ProcessThrottlerActivity): Deleted. (WebKit::ProcessThrottlerActivity::invalidate): Deleted. * Source/WebKit/UIProcess/WebProcessPool.cpp: (WebKit::WebProcessPool::updateAudibleMediaAssertions): * Source/WebKit/UIProcess/WebProcessProxy.cpp: (WebKit::WebProcessProxy::didFinishLaunching): (WebKit::WebProcessProxy::updateAudibleMediaAssertions): Canonical link: https://commits.webkit.org/270212@main

webkit-commit-queue · 2023-11-03T22:58:55Z

Committed 270212@main (5d6df46): https://commits.webkit.org/270212@main

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

pvollan requested a review from cdumez as a code owner October 27, 2023 20:00

pvollan self-assigned this Oct 27, 2023

pvollan added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Oct 27, 2023

pvollan marked this pull request as draft October 27, 2023 20:01

pvollan requested a review from brentfulgham October 27, 2023 20:01

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Oct 27, 2023

pvollan removed the merging-blocked Applied to prevent a change from being merged label Nov 1, 2023

pvollan force-pushed the eng/Minor-refactoring-of-assertion-code-v2 branch from e20e1f2 to 6bc0f8e Compare November 1, 2023 21:59

pvollan force-pushed the eng/Minor-refactoring-of-assertion-code-v2 branch from 6bc0f8e to 8963472 Compare November 1, 2023 22:03

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 1, 2023

pvollan removed the merging-blocked Applied to prevent a change from being merged label Nov 1, 2023

pvollan force-pushed the eng/Minor-refactoring-of-assertion-code-v2 branch from 8963472 to b19e985 Compare November 1, 2023 23:01

brentfulgham approved these changes Nov 2, 2023

View reviewed changes

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 2, 2023

pvollan removed the merging-blocked Applied to prevent a change from being merged label Nov 2, 2023

pvollan force-pushed the eng/Minor-refactoring-of-assertion-code-v2 branch from b19e985 to 90fb10a Compare November 2, 2023 14:25

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 2, 2023

pvollan removed the merging-blocked Applied to prevent a change from being merged label Nov 3, 2023

pvollan force-pushed the eng/Minor-refactoring-of-assertion-code-v2 branch from 90fb10a to 6f8231b Compare November 3, 2023 19:09

pvollan added the merge-queue Applied to send a pull request to merge-queue label Nov 3, 2023

Ahmad-S792 marked this pull request as ready for review November 3, 2023 22:05

webkit-commit-queue force-pushed the eng/Minor-refactoring-of-assertion-code-v2 branch from 6f8231b to 5d6df46 Compare November 3, 2023 22:58

webkit-commit-queue merged commit 5d6df46 into WebKit:main Nov 3, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minor refactoring of assertion code, v2 #19650

Minor refactoring of assertion code, v2 #19650

pvollan commented Oct 27, 2023 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Oct 27, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 1, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 1, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 1, 2023 •

edited

Loading

brentfulgham left a comment

brentfulgham Nov 1, 2023

brentfulgham Nov 1, 2023

brentfulgham Nov 1, 2023

brentfulgham Nov 1, 2023

pvollan Nov 2, 2023

brentfulgham Nov 2, 2023

brentfulgham Nov 2, 2023

pvollan Nov 2, 2023

webkit-early-warning-system commented Nov 2, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 3, 2023 •

edited

Loading

webkit-commit-queue commented Nov 3, 2023

		#endif
		static Ref<ProcessAssertion> create(ProcessID, const String& reason, ProcessAssertionType, Mode = Mode::Async, const String& environmentIdentifier = emptyString(), CompletionHandler<void()>&& acquisisionHandler = nullptr);

Minor refactoring of assertion code, v2 #19650

Minor refactoring of assertion code, v2 #19650

Conversation

pvollan commented Oct 27, 2023 • edited by webkit-early-warning-system Loading

webkit-early-warning-system commented Oct 27, 2023 • edited Loading

webkit-early-warning-system commented Nov 1, 2023 • edited Loading

webkit-early-warning-system commented Nov 1, 2023 • edited Loading

webkit-early-warning-system commented Nov 1, 2023 • edited Loading

brentfulgham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Nov 2, 2023 • edited Loading

webkit-early-warning-system commented Nov 3, 2023 • edited Loading

webkit-commit-queue commented Nov 3, 2023

pvollan commented Oct 27, 2023 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Oct 27, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 1, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 1, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 1, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 2, 2023 •

edited

Loading

webkit-early-warning-system commented Nov 3, 2023 •

edited

Loading