Skip to content

Reduce unsafeness in PDF and assorted code #47174

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

@annevk annevk self-assigned this Jun 25, 2025
@annevk annevk added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Jun 25, 2025
@webkit-ews-buildbot
Copy link
Collaborator

Safer C++ Build #41529 (859392c)

⚠️ Found 6 fixed files! Please update expectations in Source/[Project]/SaferCPPExpectations by running the following command and update your pull request:

  • Tools/Scripts/update-safer-cpp-expectations -p WebKit --UncheckedCallArgsChecker WebProcess/Plugins/PDF/PDFPluginPasswordField.mm UIProcess/mac/ViewGestureControllerMac.mm WebProcess/Plugins/PDF/PDFPluginAnnotation.mm WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm --UncountedCallArgsChecker WebProcess/Plugins/PDF/PDFPluginPasswordField.mm WebProcess/Plugins/PDF/PDFPluginAnnotation.mm WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm WebProcess/Plugins/PDF/UnifiedPDF/PDFDataDetectorOverlayController.mm UIProcess/mac/ViewGestureControllerMac.mm WebProcess/Plugins/PluginView.cpp

@annevk annevk marked this pull request as ready for review June 25, 2025 11:38
@annevk annevk requested review from cdumez and rniwa as code owners June 25, 2025 11:38
@@ -71,7 +71,7 @@
return m_plugin.get();
}

PageOverlay& PDFDataDetectorOverlayController::installOverlayIfNeeded()
Ref<PageOverlay> PDFDataDetectorOverlayController::installProtectedOverlayIfNeeded()
Copy link
Contributor

Choose a reason for hiding this comment

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

Which is this returning a Ref<>? We are not transferring ownership. This doesn't not look like a step in the right direction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is to address https://build.webkit.org/results/Apple-Sequoia-Safer-CPP-Checks/296659%40main%20%284013%29/scan-build-output/StaticAnalyzer/WebKit/StaticAnalyzerReports/report-PDFDataDetectorOverlayController.mm-handleMouseEvent-44-d83ac6.html#EndPath

I could also make the protected variant wrap the non-protected variant, but I thought that if there's only one caller that might be a bit redundant?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you added protected in the name! I missed that during review. This is fine then.

@annevk annevk force-pushed the eng/Reduce-unsafeness-in-PDF-and-assorted-code branch from 859392c to 4bf284a Compare June 27, 2025 06:49
@webkit-ews-buildbot
Copy link
Collaborator

Safer C++ Build #41792 (4bf284a)

❌ Found 1 failing file with 4 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
(cc @rniwa)

@annevk annevk force-pushed the eng/Reduce-unsafeness-in-PDF-and-assorted-code branch from 4bf284a to 259ed3d Compare June 27, 2025 09:17
@annevk annevk added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Jun 27, 2025
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Reduce-unsafeness-in-PDF-and-assorted-code branch from 259ed3d to e50830e Compare June 27, 2025 11:25
@webkit-commit-queue
Copy link
Collaborator

Committed 296716@main (e50830e): https://commits.webkit.org/296716@main

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

@webkit-commit-queue webkit-commit-queue merged commit e50830e into WebKit:main Jun 27, 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 27, 2025
@annevk annevk deleted the eng/Reduce-unsafeness-in-PDF-and-assorted-code branch June 27, 2025 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants