Adopt smart pointers in WebFullScreenManagerProxy.cpp, WebInspectorClient.cpp, WebInspectorInternal.cpp #47411

jelee53 · 2025-06-30T23:58:56Z

`51d2c6a`

Adopt smart pointers in WebFullScreenManagerProxy.cpp, WebInspectorClient.cpp, WebInspectorInternal.cpp
https://bugs.webkit.org/show_bug.cgi?id=295243
rdar://154710862

Reviewed by NOBODY (OOPS!).

Smart pointer adoption as per the static analyzer.

* Source/WebKit/SaferCPPExpectations/UncheckedCallArgsCheckerExpectations:
* Source/WebKit/SaferCPPExpectations/UncountedCallArgsCheckerExpectations:
* Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp:
(WebKit::WebFullScreenManagerProxy::isFullScreen):
* Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp:
(WebKit::WebInspectorClient::~WebInspectorClient):
(WebKit::WebInspectorClient::openLocalFrontend):
(WebKit::WebInspectorClient::bringFrontendToFront):
(WebKit::WebInspectorClient::didResizeMainFrame):
(WebKit::WebInspectorClient::showPaintRect):
(WebKit::WebInspectorClient::elementSelectionChanged):
(WebKit::WebInspectorClient::timelineRecordingChanged):
(WebKit::WebInspectorClient::setDeveloperPreferenceOverride):
* Source/WebKit/WebProcess/Inspector/WebInspectorInternal.cpp:
(WebKit::WebInspector::~WebInspector):
(WebKit::WebInspector::openLocalInspectorFrontend):
(WebKit::WebInspector::closeFrontendConnection):
(WebKit::WebInspector::bringToFront):
(WebKit::WebInspector::showMainResourceForFrame):
(WebKit::WebInspector::elementSelectionChanged):
(WebKit::WebInspector::timelineRecordingChanged):
(WebKit::WebInspector::setDeveloperPreferenceOverride):
(WebKit::WebInspector::setEmulatedConditions):
(WebKit::WebInspector::canAttachWindow):
(WebKit::WebInspector::updateDockingAvailability):
* Source/WebKit/WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::protectedInspector):
(WebKit::WebPage::protectedInspectorUI):
(WebKit::WebPage::protectedRemoteInspectorUI):
* Source/WebKit/WebProcess/WebPage/WebPage.h:

51d2c6a

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	~~🛠 playstation~~
	✅ 🛠 tv	❌ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-06-30T23:59:03Z

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

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	~~🛠 playstation~~
	✅ 🛠 tv	❌ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-ews-buildbot · 2025-07-01T01:10:14Z

Safer C++ Build #42166 (`ee580ee`)

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

jelee53 · 2025-07-01T03:21:58Z

Didn't check all the failures in the analyzer results. They were spread out across the results and I didn't realize. Will address tomorrow.

annevk · 2025-07-01T06:41:27Z

Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp

+    if (page && page->protectedInspector())
+        page->protectedInspector()->bringToFront();


Suggested change

if (page && page->protectedInspector())

page->protectedInspector()->bringToFront();

if (!page)

return;

if (RefPtr inspector = page->inspector();

inspector->bringToFront();

Is what we did elsewhere for cases like this. You should definitely never need a protected member just for a boolean output.

…ient.cpp, WebInspectorInternal.cpp https://bugs.webkit.org/show_bug.cgi?id=295243 rdar://154710862 Reviewed by NOBODY (OOPS!). Smart pointer adoption as per the static analyzer. * Source/WebKit/SaferCPPExpectations/UncheckedCallArgsCheckerExpectations: * Source/WebKit/SaferCPPExpectations/UncountedCallArgsCheckerExpectations: * Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp: (WebKit::WebFullScreenManagerProxy::isFullScreen): * Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp: (WebKit::WebInspectorClient::~WebInspectorClient): (WebKit::WebInspectorClient::openLocalFrontend): (WebKit::WebInspectorClient::bringFrontendToFront): (WebKit::WebInspectorClient::didResizeMainFrame): (WebKit::WebInspectorClient::showPaintRect): (WebKit::WebInspectorClient::elementSelectionChanged): (WebKit::WebInspectorClient::timelineRecordingChanged): (WebKit::WebInspectorClient::setDeveloperPreferenceOverride): * Source/WebKit/WebProcess/Inspector/WebInspectorInternal.cpp: (WebKit::WebInspector::~WebInspector): (WebKit::WebInspector::openLocalInspectorFrontend): (WebKit::WebInspector::closeFrontendConnection): (WebKit::WebInspector::bringToFront): (WebKit::WebInspector::showMainResourceForFrame): (WebKit::WebInspector::elementSelectionChanged): (WebKit::WebInspector::timelineRecordingChanged): (WebKit::WebInspector::setDeveloperPreferenceOverride): (WebKit::WebInspector::setEmulatedConditions): (WebKit::WebInspector::canAttachWindow): (WebKit::WebInspector::updateDockingAvailability): * Source/WebKit/WebProcess/WebPage/WebPage.cpp: (WebKit::WebPage::protectedInspector): (WebKit::WebPage::protectedInspectorUI): (WebKit::WebPage::protectedRemoteInspectorUI): * Source/WebKit/WebProcess/WebPage/WebPage.h:

webkit-early-warning-system · 2025-07-01T09:22:45Z

EWS run on current version of this PR (hash 51d2c6a)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt		✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	~~🛠 playstation~~
	✅ 🛠 tv	❌ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

jelee53 · 2025-07-01T09:26:49Z

Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp

@@ -103,22 +103,28 @@ void WebInspectorClient::frontendCountChanged(unsigned count)
 Inspector::FrontendChannel* WebInspectorClient::openLocalFrontend(InspectorController* controller)
 {
    if (RefPtr page = m_page.get())
-        page->inspector()->openLocalInspectorFrontend();
+        page->protectedInspector()->openLocalInspectorFrontend();


I'm keeping around the protectedInspector() and other protected getters just in case others might need them. I can remove though if we think just one usage in the code doesn't require a helper.

Thoughts? @annevk @pvollan

Given it's on page() I think that's warranted, might be worth a quick grep to confirm, but seems likely it's needed elsewhere.

webkit-ews-buildbot · 2025-07-01T10:53:12Z

Safer C++ Build #42214 (`51d2c6a`)

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

annevk · 2025-07-01T12:21:40Z

Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp

@@ -189,13 +195,13 @@ void WebInspectorClient::showPaintRect(const FloatRect& rect)

    if (!m_paintRectOverlay) {
        m_paintRectOverlay = PageOverlay::create(*this, PageOverlay::OverlayType::Document);
-        page->corePage()->pageOverlayController().installPageOverlay(*m_paintRectOverlay, PageOverlay::FadeMode::DoNotFade);
+        page->corePage()->pageOverlayController().installPageOverlay(*(m_paintRectOverlay.copyRef()), PageOverlay::FadeMode::DoNotFade);


Given that you need m_paintRectOverlay again below I'd write this as:

RefPtr paintRectOverlay = m_paintRectOverlay; if (!paintRectOverlay) paintRectOverlay = ... m_paintRectOverlay = paintRectOverlay.copyRef(); ... ... paintRectOverlay->layer();

I think that ends up being clearer overall.

annevk

Modulo m_paintRectOverlay nits this looks good to me. (Which I see now are also being flagged by the analyzer as wrong. Oops.)

annevk · 2025-07-01T12:22:49Z

Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp

@@ -80,7 +80,7 @@ WebInspectorClient::~WebInspectorClient()
    if (m_paintRectOverlay) {
        RefPtr page = m_page.get();
        if (page && page->corePage())
-            page->corePage()->pageOverlayController().uninstallPageOverlay(*m_paintRectOverlay, PageOverlay::FadeMode::Fade);
+            page->corePage()->pageOverlayController().uninstallPageOverlay(*(m_paintRectOverlay.copyRef()), PageOverlay::FadeMode::Fade);


You should obtain a RefPtr to m_paintRectOverlay as part of the conditional above and deref that here.

jelee53 self-assigned this Jun 30, 2025

jelee53 added the WebKit Misc. For miscellaneous bugs in the WebKit framework (and not JavaScriptCore or WebCore). label Jun 30, 2025

jelee53 requested review from pvollan and annevk July 1, 2025 00:10

annevk reviewed Jul 1, 2025

View reviewed changes

jelee53 force-pushed the eng/Adopt-smart-pointers-in-WebFullScreenManagerProxy-cpp-WebInspectorClient-cpp-WebInspectorInternal-cpp branch from ee580ee to 51d2c6a Compare July 1, 2025 09:22

jelee53 commented Jul 1, 2025

View reviewed changes

annevk reviewed Jul 1, 2025

View reviewed changes

annevk approved these changes Jul 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adopt smart pointers in WebFullScreenManagerProxy.cpp, WebInspectorClient.cpp, WebInspectorInternal.cpp #47411

Adopt smart pointers in WebFullScreenManagerProxy.cpp, WebInspectorClient.cpp, WebInspectorInternal.cpp #47411

jelee53 commented Jun 30, 2025 •

edited by webkit-early-warning-system

Loading

Uh oh!

webkit-early-warning-system commented Jun 30, 2025 •

edited

Loading

Uh oh!

webkit-ews-buildbot commented Jul 1, 2025

Uh oh!

jelee53 commented Jul 1, 2025

Uh oh!

annevk Jul 1, 2025

Uh oh!

webkit-early-warning-system commented Jul 1, 2025 •

edited

Loading

Uh oh!

jelee53 Jul 1, 2025 •

edited

Loading

Uh oh!

annevk Jul 1, 2025

Uh oh!

webkit-ews-buildbot commented Jul 1, 2025

Uh oh!

annevk Jul 1, 2025

Uh oh!

annevk left a comment •

edited

Loading

Uh oh!

annevk Jul 1, 2025

Uh oh!

Uh oh!

		if (page && page->protectedInspector())
		page->protectedInspector()->bringToFront();

-    if (page && page->protectedInspector())
-        page->protectedInspector()->bringToFront();
+    if (!page)
+        return;
+    if (RefPtr inspector = page->inspector();
+        inspector->bringToFront();

Adopt smart pointers in WebFullScreenManagerProxy.cpp, WebInspectorClient.cpp, WebInspectorInternal.cpp #47411

Are you sure you want to change the base?

Adopt smart pointers in WebFullScreenManagerProxy.cpp, WebInspectorClient.cpp, WebInspectorInternal.cpp #47411

Conversation

jelee53 commented Jun 30, 2025 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

51d2c6a

Uh oh!

webkit-early-warning-system commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-ews-buildbot commented Jul 1, 2025

Safer C++ Build #42166 (ee580ee)

Uh oh!

jelee53 commented Jul 1, 2025

Uh oh!

annevk Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelee53 Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annevk Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

webkit-ews-buildbot commented Jul 1, 2025

Safer C++ Build #42214 (51d2c6a)

Uh oh!

annevk Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

annevk left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annevk Jul 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jelee53 commented Jun 30, 2025 •

edited by webkit-early-warning-system

Loading

`51d2c6a`

webkit-early-warning-system commented Jun 30, 2025 •

edited

Loading

Safer C++ Build #42166 (`ee580ee`)

webkit-early-warning-system commented Jul 1, 2025 •

edited

Loading

jelee53 Jul 1, 2025 •

edited

Loading

Safer C++ Build #42214 (`51d2c6a`)

annevk left a comment •

edited

Loading