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

Disable building XPC services when launching WebKit processes as extensions #22660

Conversation

pvollan
Copy link
Contributor

@pvollan pvollan commented Jan 11, 2024

a945ee0

Disable building XPC services when launching WebKit processes as extensions
https://bugs.webkit.org/show_bug.cgi?id=267248
rdar://120677588

Reviewed by Elliot Williams, Brent Fulgham and Timothy Hatcher.

Disable building XPC services when launching WebKit processes as extensions. Additonally, this patch
is changing the bundle ID of the extensions to match the XPC service bundle IDs. This is to ensure
that existing bundle ID checks will continue to work. Also disable fallback to XPC service launch,
since these are not being built anymore. Instead, add a temporary fallback using the old extension
bundle IDs, since they may be cached on the system.

This patch is relanding https://commits.webkit.org/272887@main and
https://commits.webkit.org/272868@main.

* Source/WebKit/Configurations/BaseExtension.xcconfig:
* Source/WebKit/Configurations/BaseXPCService.xcconfig:
* Source/WebKit/Configurations/GPUExtension.xcconfig:
* Source/WebKit/Configurations/GPUService.xcconfig:
* Source/WebKit/Configurations/NetworkService.xcconfig:
* Source/WebKit/Configurations/NetworkingExtension.xcconfig:
* Source/WebKit/Configurations/WebContentCaptivePortalExtension.xcconfig:
* Source/WebKit/Configurations/WebContentExtension.xcconfig:
* Source/WebKit/Configurations/WebContentService.xcconfig:
* Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:
(WebKit::ProcessLauncher::setIsRetryingLaunch):
(WebKit::ProcessLauncher::isRetryingLaunch const):
* Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm:
(WebKit::serviceNameAndIdentifier):
(WebKit::launchWithExtensionKit):
(WebKit::ProcessLauncher::launchProcess):
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:

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

9a3d7f9

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

@pvollan pvollan self-assigned this Jan 11, 2024
@pvollan pvollan added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Jan 11, 2024
@pvollan pvollan force-pushed the eng/Disable-building-XPC-services-when-launching-WebKit-processes-as-extensions branch from bc62f0a to 5102752 Compare January 11, 2024 15:32
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 11, 2024
@@ -39,7 +39,9 @@ PROCESSED_INFOPLIST="${BUILT_PRODUCTS_DIR}/${INFOPLIST_PATH}"
UNPROCESSED_INFOPLIST="${INFOPLIST_FILE}"
COPIED_INFOPLIST="${WEB_CONTENT_RESOURCES_PATH}/Info-WebContent.plist"
echo "Copying Info.plist from ${UNPROCESSED_INFOPLIST} to ${COPIED_INFOPLIST}"
Copy link
Contributor

Choose a reason for hiding this comment

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

You should move this line inside the if block too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point, will fix.

Thanks for reviewing!

<string>com.apple.webkit.gpu.extension</string>
<string>com.apple.web-browser-engine.gpu</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these strings changing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think they were always wrong, and we used to have a post-build script that would fix them up.

Comment on lines 164 to 165
// Fallback to legacy extension identifiers
// This fallback is temporary and should be removed when possible. See rdar://120793705.
Copy link
Contributor

Choose a reason for hiding this comment

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

Make this a FIXME to remove it.

@@ -25,6 +25,7 @@

PRODUCT_NAME = com.apple.WebKit.GPU;
PRODUCT_BUNDLE_IDENTIFIER = $(PRODUCT_NAME);

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: delete these blank lines when you have a chance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

<string>com.apple.webkit.gpu.extension</string>
<string>com.apple.web-browser-engine.gpu</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think they were always wrong, and we used to have a post-build script that would fix them up.

@emw-apple
Copy link
Contributor

Also disable fallback to XPC service launch,
since these are not being built anymore. Instead, a temporary fallback using the old bundle IDs is
added, since the bundle IDs are normally cached on the system until they are re-registered.

I think this is referring to behavior from reverted code. It might be best to just say that it adds a temporary fallback using the old extension bundle IDs, since they may be cached on the system.

Copy link
Contributor

@emw-apple emw-apple left a comment

Choose a reason for hiding this comment

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

Marking r- because we can't land this until we make internal changes to delete existing XPC services when installing roots. See #22643 (review) for my reasoning.

@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Jan 12, 2024
@pvollan pvollan force-pushed the eng/Disable-building-XPC-services-when-launching-WebKit-processes-as-extensions branch from 5102752 to e3e8d35 Compare January 12, 2024 19:18
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for e3e8d35. Live statuses available at the PR page, #22660

@pvollan pvollan force-pushed the eng/Disable-building-XPC-services-when-launching-WebKit-processes-as-extensions branch from e3e8d35 to 9c99841 Compare January 12, 2024 19:34
@emw-apple
Copy link
Contributor

Has anything changed since my last review, or was this just a rebase?

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 13, 2024
@pvollan pvollan removed the merging-blocked Applied to prevent a change from being merged label Jan 15, 2024
@pvollan pvollan force-pushed the eng/Disable-building-XPC-services-when-launching-WebKit-processes-as-extensions branch from 9c99841 to e921936 Compare January 15, 2024 14:45
@pvollan
Copy link
Contributor Author

pvollan commented Jan 15, 2024

it adds a temporary fallback using the old extension bundle IDs, since they may be cached on the system.

Fixed!

@pvollan pvollan force-pushed the eng/Disable-building-XPC-services-when-launching-WebKit-processes-as-extensions branch from e921936 to 3be115d Compare January 15, 2024 14:51
@pvollan
Copy link
Contributor Author

pvollan commented Jan 15, 2024

Has anything changed since my last review, or was this just a rebase?

Yes, I have verified that this patch is compatible with newer and older OS builds.

@pvollan pvollan force-pushed the eng/Disable-building-XPC-services-when-launching-WebKit-processes-as-extensions branch from 3be115d to 4d016d7 Compare January 15, 2024 15:48
@pvollan pvollan force-pushed the eng/Disable-building-XPC-services-when-launching-WebKit-processes-as-extensions branch from 4d016d7 to 9a3d7f9 Compare January 15, 2024 15:51
@pvollan pvollan added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jan 17, 2024
…nsions

https://bugs.webkit.org/show_bug.cgi?id=267248
rdar://120677588

Reviewed by Elliot Williams, Brent Fulgham and Timothy Hatcher.

Disable building XPC services when launching WebKit processes as extensions. Additonally, this patch
is changing the bundle ID of the extensions to match the XPC service bundle IDs. This is to ensure
that existing bundle ID checks will continue to work. Also disable fallback to XPC service launch,
since these are not being built anymore. Instead, add a temporary fallback using the old extension
bundle IDs, since they may be cached on the system.

This patch is relanding https://commits.webkit.org/272887@main and
https://commits.webkit.org/272868@main.

* Source/WebKit/Configurations/BaseExtension.xcconfig:
* Source/WebKit/Configurations/BaseXPCService.xcconfig:
* Source/WebKit/Configurations/GPUExtension.xcconfig:
* Source/WebKit/Configurations/GPUService.xcconfig:
* Source/WebKit/Configurations/NetworkService.xcconfig:
* Source/WebKit/Configurations/NetworkingExtension.xcconfig:
* Source/WebKit/Configurations/WebContentCaptivePortalExtension.xcconfig:
* Source/WebKit/Configurations/WebContentExtension.xcconfig:
* Source/WebKit/Configurations/WebContentService.xcconfig:
* Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:
(WebKit::ProcessLauncher::setIsRetryingLaunch):
(WebKit::ProcessLauncher::isRetryingLaunch const):
* Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm:
(WebKit::serviceNameAndIdentifier):
(WebKit::launchWithExtensionKit):
(WebKit::ProcessLauncher::launchProcess):
* Source/WebKit/WebKit.xcodeproj/project.pbxproj:

Canonical link: https://commits.webkit.org/273113@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Disable-building-XPC-services-when-launching-WebKit-processes-as-extensions branch from 9a3d7f9 to a945ee0 Compare January 17, 2024 05:40
@webkit-commit-queue webkit-commit-queue merged commit a945ee0 into WebKit:main Jan 17, 2024
@webkit-commit-queue
Copy link
Collaborator

Committed 273113@main (a945ee0): https://commits.webkit.org/273113@main

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

@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 Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore).
Projects
None yet
6 participants