Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass invalidation handler to ExtensionProcess::grantCapability #28731

Conversation

pvollan
Copy link
Contributor

@pvollan pvollan commented May 17, 2024

1191300

Pass invalidation handler to ExtensionProcess::grantCapability
https://bugs.webkit.org/show_bug.cgi?id=274331
rdar://126825362

Reviewed by Chris Dumez.

In order for the invalidation handler to be called when the grant is invalidated, we need to
pass the invalidation handler to ExtensionProcess::grantCapability.

* Source/WebKit/Platform/cocoa/AssertionCapability.h:
* Source/WebKit/UIProcess/Cocoa/ProcessAssertionCocoa.mm:
(WebKit::ProcessAssertion::ProcessAssertion):
(WebKit::ProcessAssertion::acquireSync):
(WebKit::Function<void):
* Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:
* Source/WebKit/UIProcess/Launcher/cocoa/ExtensionProcess.h:
* Source/WebKit/UIProcess/Launcher/cocoa/ExtensionProcess.mm:
(WebKit::ExtensionProcess::grantCapability const):
* Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm:
(WebKit::LaunchGrant::LaunchGrant):
(WebKit::LaunchGrant::~LaunchGrant):
* Source/WebKit/UIProcess/ProcessAssertion.h:

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

a4e26e4

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
  πŸ›  ios-sim βœ… πŸ›  mac-AS-debug   πŸ§ͺ wpe-wk2 βœ… πŸ§ͺ wincairo-tests
βœ… πŸ§ͺ webkitperl ❌ πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
  πŸ§ͺ ios-wk2-wpt βœ… πŸ›  wpe-cairo
  πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2   πŸ§ͺ gtk-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress   πŸ§ͺ api-gtk
βœ… πŸ›  watch
βœ… πŸ›  πŸ§ͺ unsafe-merge βœ… πŸ›  watch-sim

@pvollan pvollan requested a review from cdumez as a code owner May 17, 2024 21:16
@pvollan pvollan self-assigned this May 17, 2024
@pvollan pvollan added the WebKit Process Model Bugs related to WebKit's multi-process architecture label May 17, 2024
@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from 9a66f23 to 2b99bd1 Compare May 17, 2024 22:27
@pvollan pvollan requested a review from xeenon May 17, 2024 23:25
@@ -388,14 +388,6 @@ static ASCIILiteral runningBoardDomainForAssertionType(ProcessAssertionType asse
if (process.extensionProcess()) {
ASCIILiteral runningBoardAssertionName = runningBoardNameForAssertionType(m_assertionType);
ASCIILiteral runningBoardDomain = runningBoardDomainForAssertionType(m_assertionType);
auto didInvalidateBlock = [weakThis = ThreadSafeWeakPtr { *this }, runningBoardAssertionName] () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably so the code can be shared with the call on L477

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is correct.

Thanks for reviewing, all!

@@ -84,7 +88,7 @@
return grant;
}

PlatformGrant ExtensionProcess::grantCapability(const PlatformCapability& capability) const
PlatformGrant ExtensionProcess::grantCapability(const PlatformCapability& capability, Function<void()>&& invalidationHandler) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this take an AssertionCapability instead of a PlatformCapability as an argument and just use the m_didInvalidateBlock from AssertionCapability? It seems a bit weird to be passing the invalidation block in two different places...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point! I will make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started on this change, but it became more involving than expected, because this method is also used to grant Media capabilities. Would you be OK with the current approach, Ben?

@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from 2b99bd1 to 7f49181 Compare May 24, 2024 17:22
@pvollan pvollan requested review from cdumez and bnham May 24, 2024 17:23
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 24, 2024
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label May 24, 2024
@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from 7f49181 to 1b9e666 Compare May 24, 2024 18:25
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 24, 2024
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label May 24, 2024
@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from 1b9e666 to 4e1886e Compare May 24, 2024 19:05
@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from 4e1886e to c1f9e37 Compare May 24, 2024 19:12
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 24, 2024
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label May 24, 2024
@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from c1f9e37 to 7c77bf4 Compare May 24, 2024 20:30
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 24, 2024
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label May 25, 2024
@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from 7c77bf4 to 5bfe028 Compare May 25, 2024 00:08
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 25, 2024
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label May 25, 2024
@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from 047e663 to ce15a65 Compare May 25, 2024 15:55
@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from ce15a65 to 00abc02 Compare May 25, 2024 16:08
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 25, 2024
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label May 26, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 27, 2024
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label May 28, 2024
@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from 00abc02 to 9a60324 Compare May 28, 2024 03:06
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 28, 2024
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label May 28, 2024
@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from 9a60324 to ad2dbb8 Compare May 28, 2024 04:15
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 28, 2024
@pvollan
Copy link
Contributor Author

pvollan commented May 28, 2024

Looking closer, I think the test failures on iOS are unrelated to this patch.

@cdumez
Copy link
Contributor

cdumez commented May 28, 2024

Pass invalidation handler to ExtensionProcess::grantCapability
https://bugs.webkit.org/show_bug.cgi?id=274331
rdar://126825362

This radar is already in Integrate so I doubt this is the right link.

@pvollan
Copy link
Contributor Author

pvollan commented May 28, 2024

Pass invalidation handler to ExtensionProcess::grantCapability
https://bugs.webkit.org/show_bug.cgi?id=274331
rdar://126825362

This radar is already in Integrate so I doubt this is the right link.

Ah, good catch, the radar state was not correct.

Thanks for approving, Chris!

@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label May 29, 2024
@pvollan pvollan force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from ad2dbb8 to a4e26e4 Compare May 29, 2024 02:55
@pvollan pvollan added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 29, 2024
https://bugs.webkit.org/show_bug.cgi?id=274331
rdar://126825362

Reviewed by Chris Dumez.

In order for the invalidation handler to be called when the grant is invalidated, we need to
pass the invalidation handler to ExtensionProcess::grantCapability.

* Source/WebKit/Platform/cocoa/AssertionCapability.h:
* Source/WebKit/UIProcess/Cocoa/ProcessAssertionCocoa.mm:
(WebKit::ProcessAssertion::ProcessAssertion):
(WebKit::ProcessAssertion::acquireSync):
(WebKit::Function<void):
* Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:
* Source/WebKit/UIProcess/Launcher/cocoa/ExtensionProcess.h:
* Source/WebKit/UIProcess/Launcher/cocoa/ExtensionProcess.mm:
(WebKit::ExtensionProcess::grantCapability const):
* Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm:
(WebKit::LaunchGrant::LaunchGrant):
(WebKit::LaunchGrant::~LaunchGrant):
* Source/WebKit/UIProcess/ProcessAssertion.h:

Canonical link: https://commits.webkit.org/279428@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Pass-invalidation-handler-to-ExtensionProcessgrantCapability branch from a4e26e4 to 1191300 Compare May 29, 2024 05:30
@webkit-commit-queue
Copy link
Collaborator

Committed 279428@main (1191300): https://commits.webkit.org/279428@main

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

@webkit-commit-queue webkit-commit-queue merged commit 1191300 into WebKit:main May 29, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit Process Model Bugs related to WebKit's multi-process architecture
Projects
None yet
6 participants