Skip to content

Use CheckedPtr / CheckedRef less for non-stack objects#21886

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
cdumez:266503_less_CheckedPtr
Dec 16, 2023
Merged

Use CheckedPtr / CheckedRef less for non-stack objects#21886
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
cdumez:266503_less_CheckedPtr

Conversation

@cdumez
Copy link
Copy Markdown
Contributor

@cdumez cdumez commented Dec 15, 2023

98667dc

Use CheckedPtr / CheckedRef less for non-stack objects
https://bugs.webkit.org/show_bug.cgi?id=266503

Reviewed by Darin Adler and Ryosuke Niwa.

* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AccessibilityScrollView.h:
* Source/WebCore/loader/ApplicationManifestLoader.h:
* Source/WebCore/loader/DocumentLoader.h:
* Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:
(WebCore::ApplicationCacheHost::ApplicationCacheHost):
* Source/WebCore/loader/appcache/ApplicationCacheHost.h:
* Source/WebCore/loader/cache/CachedResourceLoader.h:
* Source/WebCore/loader/icon/IconLoader.h:
* Source/WebCore/page/DOMWindowExtension.h:
* Source/WebCore/page/EventHandler.cpp:
(WebCore::widgetForElement):
(WebCore::EventHandler::completeWidgetWheelEvent):
(WebCore::EventHandler::handleWheelEventInternal):
* Source/WebCore/page/EventHandler.h:
* Source/WebCore/page/LocalFrameView.cpp:
(WebCore::LocalFrameView::didAddWidgetToRenderTree):
(WebCore::LocalFrameView::willRemoveWidgetFromRenderTree):
(WebCore::collectAndProtectWidgets):
(WebCore::LocalFrameView::updateWidgetPositions):
* Source/WebCore/page/LocalFrameView.h:
* Source/WebCore/page/PageOverlay.h:
* Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:
(WebCore::ScrollingCoordinator::absoluteEventTrackingRegionsForFrame const):
* Source/WebCore/platform/ScrollView.h:
* Source/WebCore/platform/Widget.h:
* Source/WebCore/platform/mac/ScrollbarThemeMac.mm:
(WebCore::ScrollbarThemeMac::registerScrollbar):
(WebCore::ScrollbarThemeMac::unregisterScrollbar):
* Source/WebCore/platform/mac/ScrollbarsControllerMac.mm:
* Source/WebCore/rendering/CounterNode.cpp:
(WebCore::CounterNode::~CounterNode):
(WebCore::CounterNode::nextInPreOrderAfterChildren const):
(WebCore::CounterNode::lastDescendant const):
(WebCore::CounterNode::previousInPreOrder const):
(WebCore::CounterNode::resetThisAndDescendantsRenderers):
(WebCore::CounterNode::recount):
(WebCore::CounterNode::insertAfter):
(WebCore::CounterNode::removeChild):
* Source/WebCore/rendering/CounterNode.h:
* Source/WebCore/rendering/RenderCounter.cpp:
(WebCore::makeCounterNode):
(WebCore::RenderCounter::originalText const):
* Source/WebCore/rendering/RenderCounter.h:
* Source/WebKit/WebProcess/InjectedBundle/InjectedBundleDOMWindowExtension.cpp:
(WebKit::InjectedBundleDOMWindowExtension::InjectedBundleDOMWindowExtension):
(WebKit::InjectedBundleDOMWindowExtension::~InjectedBundleDOMWindowExtension):
* Source/WebKit/WebProcess/InjectedBundle/InjectedBundleDOMWindowExtension.h:
* Source/WebKit/WebProcess/Plugins/PDF/PDFPluginBase.h:
* Source/WebKit/WebProcess/WebPage/WebPage.h:
* Source/WebKit/WebProcess/WebPage/WebPageOverlay.cpp:
(WebKit::overlayMap):
(WebKit::WebPageOverlay::WebPageOverlay):
(WebKit::WebPageOverlay::~WebPageOverlay):
(WebKit::WebPageOverlay::fromCoreOverlay):
* Source/WebKit/WebProcess/WebPage/WebPageOverlay.h:

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

3685f97

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 ✅ 🛠 gtk
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@cdumez cdumez self-assigned this Dec 15, 2023
@cdumez cdumez added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Dec 15, 2023
@cdumez cdumez marked this pull request as ready for review December 16, 2023 01:11
Copy link
Copy Markdown
Member

@darinadler darinadler left a comment

Choose a reason for hiding this comment

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

Is it safe to use weak pointers as hash table keys?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need for the get() now that it’s a RefPtr.

@cdumez
Copy link
Copy Markdown
Contributor Author

cdumez commented Dec 16, 2023

Is it safe to use weak pointers as hash table keys?

I made sure that WeakRef<> works fine as hash table key. Of course, it will crash if the object gets destroyed but it should crash safely:

  1. Any access to a WeakRef will do a null deref once the object has been destroyed
  2. The hash table null / empty values for WeakRef differ from a WeakRef that's been nulled out due to object destroyed so it shouldn't confuse the HashTable implementation.

@cdumez cdumez force-pushed the 266503_less_CheckedPtr branch from 95a1454 to 3685f97 Compare December 16, 2023 02:18
@cdumez cdumez added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Dec 16, 2023
https://bugs.webkit.org/show_bug.cgi?id=266503

Reviewed by Darin Adler and Ryosuke Niwa.

* Source/WebCore/accessibility/AXObjectCache.h:
* Source/WebCore/accessibility/AccessibilityScrollView.h:
* Source/WebCore/loader/ApplicationManifestLoader.h:
* Source/WebCore/loader/DocumentLoader.h:
* Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:
(WebCore::ApplicationCacheHost::ApplicationCacheHost):
* Source/WebCore/loader/appcache/ApplicationCacheHost.h:
* Source/WebCore/loader/cache/CachedResourceLoader.h:
* Source/WebCore/loader/icon/IconLoader.h:
* Source/WebCore/page/DOMWindowExtension.h:
* Source/WebCore/page/EventHandler.cpp:
(WebCore::widgetForElement):
(WebCore::EventHandler::completeWidgetWheelEvent):
(WebCore::EventHandler::handleWheelEventInternal):
* Source/WebCore/page/EventHandler.h:
* Source/WebCore/page/LocalFrameView.cpp:
(WebCore::LocalFrameView::didAddWidgetToRenderTree):
(WebCore::LocalFrameView::willRemoveWidgetFromRenderTree):
(WebCore::collectAndProtectWidgets):
(WebCore::LocalFrameView::updateWidgetPositions):
* Source/WebCore/page/LocalFrameView.h:
* Source/WebCore/page/PageOverlay.h:
* Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:
(WebCore::ScrollingCoordinator::absoluteEventTrackingRegionsForFrame const):
* Source/WebCore/platform/ScrollView.h:
* Source/WebCore/platform/Widget.h:
* Source/WebCore/platform/mac/ScrollbarThemeMac.mm:
(WebCore::ScrollbarThemeMac::registerScrollbar):
(WebCore::ScrollbarThemeMac::unregisterScrollbar):
* Source/WebCore/platform/mac/ScrollbarsControllerMac.mm:
* Source/WebCore/rendering/CounterNode.cpp:
(WebCore::CounterNode::~CounterNode):
(WebCore::CounterNode::nextInPreOrderAfterChildren const):
(WebCore::CounterNode::lastDescendant const):
(WebCore::CounterNode::previousInPreOrder const):
(WebCore::CounterNode::resetThisAndDescendantsRenderers):
(WebCore::CounterNode::recount):
(WebCore::CounterNode::insertAfter):
(WebCore::CounterNode::removeChild):
* Source/WebCore/rendering/CounterNode.h:
* Source/WebCore/rendering/RenderCounter.cpp:
(WebCore::makeCounterNode):
(WebCore::RenderCounter::originalText const):
* Source/WebCore/rendering/RenderCounter.h:
* Source/WebKit/WebProcess/InjectedBundle/InjectedBundleDOMWindowExtension.cpp:
(WebKit::InjectedBundleDOMWindowExtension::InjectedBundleDOMWindowExtension):
(WebKit::InjectedBundleDOMWindowExtension::~InjectedBundleDOMWindowExtension):
* Source/WebKit/WebProcess/InjectedBundle/InjectedBundleDOMWindowExtension.h:
* Source/WebKit/WebProcess/Plugins/PDF/PDFPluginBase.h:
* Source/WebKit/WebProcess/WebPage/WebPage.h:
* Source/WebKit/WebProcess/WebPage/WebPageOverlay.cpp:
(WebKit::overlayMap):
(WebKit::WebPageOverlay::WebPageOverlay):
(WebKit::WebPageOverlay::~WebPageOverlay):
(WebKit::WebPageOverlay::fromCoreOverlay):
* Source/WebKit/WebProcess/WebPage/WebPageOverlay.h:

Canonical link: https://commits.webkit.org/272165@main
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 272165@main (98667dc): https://commits.webkit.org/272165@main

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

@webkit-commit-queue webkit-commit-queue merged commit 98667dc into WebKit:main Dec 16, 2023
@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 Dec 16, 2023
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.

5 participants