Adopt more smart pointers in History.cpp #27975

rwlbuis · 2024-05-01T09:06:18Z

`cde4840`

Adopt more smart pointers in History.cpp
https://bugs.webkit.org/show_bug.cgi?id=273541

Reviewed by Sihui Liu.

Adopt more smart pointers in History.cpp based on the
[alpha.webkit.UncountedLocalVarsChecker] warning.

* Source/WebCore/page/History.cpp:
(WebCore::History::length const):
(WebCore::History::scrollRestoration const):
(WebCore::History::setScrollRestoration):
(WebCore::History::go):
(WebCore::History::urlForState):

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

3ecb068

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-05-01T09:06:23Z

EWS run on previous version of this PR (hash 6d52e3f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	❌ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	~~🧪 api-mac~~	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	✅ 🛠 wpe-skia
	⏳ 🧪 api-ios	~~🧪 mac-wk2~~	✅ 🛠 gtk
	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	~~🧪 gtk-wk2~~
	✅ 🛠 tv-sim	~~🧪 mac-wk2-stress~~	~~🧪 api-gtk~~
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-05-01T09:23:14Z

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

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	⏳ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	⏳ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-05-01T15:58:48Z

EWS run on previous version of this PR (hash 9faab32)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	~~🧪 api-mac~~	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-05-01T21:40:07Z

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

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	❌ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2024-05-02T13:07:49Z

EWS run on previous version of this PR (hash 04d2614)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	⏳ 🧪 api-gtk
	⏳ 🛠 watch
	✅ 🛠 watch-sim

cdumez · 2024-05-02T14:41:13Z

Source/WebCore/page/History.cpp

        return documentNotFullyActive();

-    auto* historyItem = frame->history().currentItem();
+    SUPPRESS_UNCOUNTED_LOCAL auto* historyItem = frame->history().currentItem();


We shouldn't add SUPPRESS_UNCOUNTED_LOCAL I think

cdumez · 2024-05-02T14:41:29Z

Source/WebCore/page/History.cpp

        return documentNotFullyActive();
    m_lastStateObjectRequested = stateInternal();
    return m_lastStateObjectRequested.get();
 }

 SerializedScriptValue* History::stateInternal() const
 {
-    auto* frame = this->frame();
+    SUPPRESS_UNCOUNTED_LOCAL auto* frame = this->frame();


cdumez · 2024-05-02T14:41:33Z

Source/WebCore/page/History.cpp

    if (!frame)
        return nullptr;
-    auto* historyItem = frame->history().currentItem();
+    SUPPRESS_UNCOUNTED_LOCAL auto* historyItem = frame->history().currentItem();


Also removed.

szewai · 2024-05-02T16:48:47Z

Source/WebCore/page/History.cpp

        return frame->document()->url();
    return frame->document()->completeURL(urlString);


I think we need to null check document (or assert) here, and protect it before invoking completeURL.

Good point! I now use protectedDocument. For urlForState I removed reffing of the frame since the (only) caller (stateObjectAdded) already has a ref to the frame.

webkit-early-warning-system · 2024-05-02T18:10:36Z

EWS run on previous version of this PR (hash 4311ea4)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	⏳ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	🧪 api-mac	⏳ 🧪 api-wpe
	⏳ 🧪 ios-wk2-wpt	~~🧪 mac-wk1~~	✅ 🛠 wpe-skia
⏳ 🛠 🧪 jsc	⏳ 🧪 api-ios	~~🧪 mac-wk2~~	✅ 🛠 gtk
🛠 🧪 jsc-arm64	⏳ 🛠 tv	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 gtk-wk2
	⏳ 🛠 tv-sim	~~🧪 mac-wk2-stress~~	⏳ 🧪 api-gtk
	⏳ 🛠 watch
	🛠 watch-sim

webkit-early-warning-system · 2024-05-02T18:24:18Z

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

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	⏳ 🛠 ios	🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	🛠 mac-AS-debug	✅ 🧪 wpe-wk2
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	⏳ 🛠 wpe-skia
	🧪 api-ios	✅ 🧪 mac-wk2	⏳ 🛠 gtk
	✅ 🛠 tv	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	🛠 watch		⏳ 🛠 jsc-armv7
	✅ 🛠 watch-sim		⏳ 🧪 jsc-armv7-tests

szewai · 2024-05-02T21:57:48Z

Source/WebCore/page/History.cpp

        return documentNotFullyActive();

-    auto* historyItem = frame->history().currentItem();
+    RefPtr historyItem = frame->history().currentItem();


Seems this can be simplified as:

if (RefPtr historyItem = frame->history().currentItem()) historyItem->setShouldRestoreScrollPosition(scrollRestoration == ScrollRestoration::Auto);

webkit-early-warning-system · 2024-05-03T08:19:20Z

EWS run on current version of this PR (hash 3ecb068)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-skia
	~~🧪 api-ios~~	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 tv	~~🧪 mac-AS-debug-wk2~~	✅ 🧪 gtk-wk2
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2-stress	~~🧪 api-gtk~~
✅ 🛠 🧪 merge	✅ 🛠 watch
	✅ 🛠 watch-sim

https://bugs.webkit.org/show_bug.cgi?id=273541 Reviewed by Sihui Liu. Adopt more smart pointers in History.cpp based on the [alpha.webkit.UncountedLocalVarsChecker] warning. * Source/WebCore/page/History.cpp: (WebCore::History::length const): (WebCore::History::scrollRestoration const): (WebCore::History::setScrollRestoration): (WebCore::History::go): (WebCore::History::urlForState): Canonical link: https://commits.webkit.org/278310@main

webkit-commit-queue · 2024-05-03T10:09:51Z

Committed 278310@main (cde4840): https://commits.webkit.org/278310@main

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

rwlbuis requested a review from cdumez as a code owner May 1, 2024 09:06

rwlbuis self-assigned this May 1, 2024

rwlbuis added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label May 1, 2024

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 1, 2024

rwlbuis removed the merging-blocked Applied to prevent a change from being merged label May 1, 2024

rwlbuis force-pushed the eng/Adopt-more-smart-pointers-in-History-cpp branch from 6d52e3f to b7987e2 Compare May 1, 2024 09:23

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 1, 2024

rwlbuis removed the merging-blocked Applied to prevent a change from being merged label May 1, 2024

rwlbuis force-pushed the eng/Adopt-more-smart-pointers-in-History-cpp branch from b7987e2 to 9faab32 Compare May 1, 2024 15:58

rwlbuis force-pushed the eng/Adopt-more-smart-pointers-in-History-cpp branch from 9faab32 to c8e85a2 Compare May 1, 2024 21:39

rwlbuis requested review from szewai and removed request for cdumez May 1, 2024 21:40

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 2, 2024

rwlbuis removed the merging-blocked Applied to prevent a change from being merged label May 2, 2024

rwlbuis force-pushed the eng/Adopt-more-smart-pointers-in-History-cpp branch from c8e85a2 to 04d2614 Compare May 2, 2024 08:41

cdumez reviewed May 2, 2024

View reviewed changes

szewai reviewed May 2, 2024

View reviewed changes

rwlbuis force-pushed the eng/Adopt-more-smart-pointers-in-History-cpp branch from 04d2614 to 4311ea4 Compare May 2, 2024 18:10

rwlbuis force-pushed the eng/Adopt-more-smart-pointers-in-History-cpp branch from 4311ea4 to c1353ce Compare May 2, 2024 18:23

rwlbuis requested review from cdumez and szewai May 2, 2024 21:30

szewai approved these changes May 2, 2024

View reviewed changes

rwlbuis force-pushed the eng/Adopt-more-smart-pointers-in-History-cpp branch from c1353ce to 3ecb068 Compare May 3, 2024 08:19

rwlbuis added the merge-queue Applied to send a pull request to merge-queue label May 3, 2024

webkit-commit-queue force-pushed the eng/Adopt-more-smart-pointers-in-History-cpp branch from 3ecb068 to cde4840 Compare May 3, 2024 10:09

webkit-commit-queue merged commit cde4840 into WebKit:main May 3, 2024

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 3, 2024

rwlbuis deleted the eng/Adopt-more-smart-pointers-in-History-cpp branch May 3, 2024 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt more smart pointers in History.cpp #27975

Adopt more smart pointers in History.cpp #27975

rwlbuis commented May 1, 2024 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented May 1, 2024 •

edited

Loading

webkit-early-warning-system commented May 1, 2024 •

edited

Loading

webkit-early-warning-system commented May 1, 2024 •

edited

Loading

webkit-early-warning-system commented May 1, 2024 •

edited

Loading

webkit-early-warning-system commented May 2, 2024 •

edited

Loading

cdumez May 2, 2024

rwlbuis May 2, 2024

cdumez May 2, 2024

rwlbuis May 2, 2024

cdumez May 2, 2024

rwlbuis May 2, 2024

szewai May 2, 2024

rwlbuis May 2, 2024

webkit-early-warning-system commented May 2, 2024 •

edited

Loading

webkit-early-warning-system commented May 2, 2024 •

edited

Loading

szewai May 2, 2024

webkit-early-warning-system commented May 3, 2024 •

edited

Loading

webkit-commit-queue commented May 3, 2024

		return frame->document()->url();
		return frame->document()->completeURL(urlString);

Adopt more smart pointers in History.cpp #27975

Adopt more smart pointers in History.cpp #27975

Conversation

rwlbuis commented May 1, 2024 • edited by webkit-early-warning-system Loading

webkit-early-warning-system commented May 1, 2024 • edited Loading

webkit-early-warning-system commented May 1, 2024 • edited Loading

webkit-early-warning-system commented May 1, 2024 • edited Loading

webkit-early-warning-system commented May 1, 2024 • edited Loading

webkit-early-warning-system commented May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented May 2, 2024 • edited Loading

webkit-early-warning-system commented May 2, 2024 • edited Loading

Choose a reason for hiding this comment

webkit-early-warning-system commented May 3, 2024 • edited Loading

webkit-commit-queue commented May 3, 2024

rwlbuis commented May 1, 2024 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented May 1, 2024 •

edited

Loading

webkit-early-warning-system commented May 1, 2024 •

edited

Loading

webkit-early-warning-system commented May 1, 2024 •

edited

Loading

webkit-early-warning-system commented May 1, 2024 •

edited

Loading

webkit-early-warning-system commented May 2, 2024 •

edited

Loading

webkit-early-warning-system commented May 2, 2024 •

edited

Loading

webkit-early-warning-system commented May 2, 2024 •

edited

Loading

webkit-early-warning-system commented May 3, 2024 •

edited

Loading