Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Crash under WebCore: WebCore::CachedResourceClientWalker<WebCore::CachedImageClient>::next() #495

Merged
merged 0 commits into from May 5, 2022
Merged

Crash under WebCore: WebCore::CachedResourceClientWalker<WebCore::CachedImageClient>::next() #495

merged 0 commits into from May 5, 2022

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented May 4, 2022 •
edited by webkit-early-warning-system

8dc401e

Crash under WebCore: WebCore::CachedResourceClientWalker<WebCore::CachedImageClient >::next()
https://bugs.webkit.org/show_bug.cgi?id=240072
<rdar://92717726 >

Reviewed by Geoff Garen.

Have CachedResource and CachedResourceClientWalker hold the clients via WeakPtrs instead of
raw pointers and null check them before usage. This is a lot safer.

* Source/WTF/WTF.xcodeproj/project.pbxproj:
* Source/WTF/wtf/WeakHashCountedSet.h: Added.
(WTF::WeakHashCountedSet::begin):
(WTF::WeakHashCountedSet::end):
(WTF::WeakHashCountedSet::begin const):
(WTF::WeakHashCountedSet::end const):
(WTF::WeakHashCountedSet::find):
(WTF::WeakHashCountedSet::find const):
(WTF::WeakHashCountedSet::contains const):
(WTF::WeakHashCountedSet::computeSize const):
(WTF::WeakHashCountedSet::clear):
(WTF::Counter >::add):
(WTF::Counter >::remove):
* Source/WebCore/html/HTMLLinkElement.h:
* Source/WebCore/loader/DocumentThreadableLoader.h:
* Source/WebCore/loader/ImageLoader.h:
* Source/WebCore/loader/LinkLoader.h:
* Source/WebCore/loader/cache/CachedImage.cpp:
(WebCore::CachedImage::addClientWaitingForAsyncDecoding):
* Source/WebCore/loader/cache/CachedResource.cpp:
(WebCore::CachedResource::didAddClient):
(WebCore::CachedResource::addClientToSet):
(WebCore::CachedResource::removeClient):
(WebCore::CachedResource::switchClientsToRevalidatedResource):
* Source/WebCore/loader/cache/CachedResource.h:
(WebCore::CachedResource::hasClients const):
(WebCore::CachedResource::hasClient):
(WebCore::CachedResource::numberOfClients const):
* Source/WebCore/loader/cache/CachedResourceClient.h:
* Source/WebCore/loader/cache/CachedResourceClientWalker.h:
(WebCore::CachedResourceClientWalker::CachedResourceClientWalker):
(WebCore::CachedResourceClientWalker::next):
* Source/WebCore/rendering/RenderObject.h:

Canonical link: https://commits.webkit.org/250278@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@293804 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@cdumez cdumez self-assigned this May 4, 2022
@cdumez cdumez added WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). WebKit Nightly Build labels May 4, 2022
@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label May 4, 2022
@cdumez cdumez added WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). WebKit Nightly Build and removed merging-blocked Applied to prevent a change from being merged WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). WebKit Nightly Build labels May 4, 2022
geoffreygaren
geoffreygaren approved these changes May 4, 2022
View reviewed changes
Copy link
Contributor

@geoffreygaren geoffreygaren left a comment

Choose a reason for hiding this comment

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

r=me

Source/WebCore/loader/cache/CachedResource.cpp
Vector<CachedResourceClient*> clientsToMove;
for (auto& entry : m_clients) {
CachedResourceClient* client = entry.key;
Vector<WeakPtr<CachedResourceClient>> clientsToMove;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're certain that these pointers won't be deleted during this operation, then I think the best option is Vector<CachedResourceClient&>.

But I suppose that removeClient() and didAddClient() might have arbitrary side effects? If so, I guess we need to add some null checks when we call removeClient() and addClientToSet().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added null checks.

@cdumez cdumez added the merge-queue Applied to send a pull request to merge-queue label May 4, 2022
@webkit-early-warning-system webkit-early-warning-system merged commit 8dc401e into WebKit:main May 5, 2022
@webkit-early-warning-system
Copy link
Collaborator

Committed r293804 (250278@main): https://commits.webkit.org/250278@main

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

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label May 5, 2022
@cdumez cdumez deleted the 240072_CachedResourceClient_WeakPtr branch May 5, 2022 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoffreygaren geoffreygaren geoffreygaren approved these changes

Assignees

@cdumez cdumez

Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
Milestone
No milestone
3 participants
@cdumez @webkit-early-warning-system @geoffreygaren