Skip to content

Check hash map keys before adding to map #47123

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

Conversation

@pvollan pvollan requested a review from cdumez as a code owner June 24, 2025 17:22
@pvollan pvollan self-assigned this Jun 24, 2025
@pvollan pvollan added the WebKit Process Model Bugs related to WebKit's multi-process architecture label Jun 24, 2025
fontURLs.add(fontFamilyName.convertToASCIILowercase(), fontURL);
auto lowerCaseFontName = fontName.convertToASCIILowercase();
auto lowerCaseFontFamilyName = fontFamilyName.convertToASCIILowercase();
if (lowerCaseFontName.isEmpty() || lowerCaseFontFamilyName.isEmpty())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a chance one is null but not the other? And it that case, wouldn't we want to add the non-null one to the HashMap?

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!

@pvollan pvollan force-pushed the eng/Check-hash-map-keys-before-adding-to-map branch from 408c68a to d1b9e72 Compare June 24, 2025 17:30
@pvollan pvollan requested a review from cdumez June 24, 2025 17:30
fontURLs.add(fontFamilyName.convertToASCIILowercase(), fontURL);
didAddFontToMap = true;
auto lowerCaseFontName = fontName.convertToASCIILowercase();
if (!lowerCaseFontName.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this API-testable?

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 is a good point! It requires an invalid user installed font, so it may not be trivial, but it's definitely doable. If time permits, I will look into creating an API test for this.

Thanks for reviewing!

@pvollan pvollan added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 24, 2025
https://bugs.webkit.org/show_bug.cgi?id=294911
rdar://154104621

Reviewed by Chris Dumez.

* Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:
(WebKit::addUserInstalledFontURLs):

Canonical link: https://commits.webkit.org/296587@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Check-hash-map-keys-before-adding-to-map branch from d1b9e72 to b13bdfd Compare June 24, 2025 23:50
@webkit-commit-queue
Copy link
Collaborator

Committed 296587@main (b13bdfd): https://commits.webkit.org/296587@main

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

@webkit-commit-queue webkit-commit-queue merged commit b13bdfd into WebKit:main Jun 24, 2025
@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 Jun 24, 2025
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
Development

Successfully merging this pull request may close these issues.

4 participants