Summary: | [GTK] segmentation fault in WebKit::IconDatabase::loadIconForPageURL | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jim Mason <jmason> | ||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | aperez, berto, bugs-noreply, cgarcia, dennisn, ews-watchlist, gustavo, mike | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=218427 https://bugs.webkit.org/show_bug.cgi?id=225348 |
||||||||||||
Attachments: |
|
Description
Jim Mason
2021-05-02 08:17:38 PDT
Created attachment 427525 [details]
Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Created attachment 427526 [details]
Patch
Created attachment 427529 [details]
Patch
Comment on attachment 427529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=427529&action=review > Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:586 > + if (nativeImage != nullptr) Do not compare to nullptr. We normally prefer the early return first and avoid the else, so this would be: if (!nativeImage) { completionHandler(nullptr); return; } addResult.iterator->value.first = nativeImage->platformImage(); > Source/WebKit/UIProcess/API/glib/IconDatabase.cpp:626 > + auto nativeImage = image->nativeImageForCurrentFrame(); > + if (nativeImage != nullptr) And here this could be if (auto nativeImage = image->nativeImageForCurrentFrame()) *** Bug 225313 has been marked as a duplicate of this bug. *** Thanks for the feedback. In setIconForPageURL, I am tempted to simplify this: auto image = BitmapImage::create(); if (image->setData(SharedBuffer::create(iconData, iconDataSize), true) < EncodedDataStatus::SizeAvailable) result = false; else if (auto nativeImage = image->nativeImageForCurrentFrame()) addResult.iterator->value.first = nativeImage->platformImage(); else result = false; to something like this: RefPtr<NativeImage> nativeImage; auto image = BitmapImage::create(); if (image->setData(SharedBuffer::create(iconData, iconDataSize), true) >= EncodedDataStatus::SizeAvailable && (nativeImage = image->nativeImageForCurrentFrame())) addResult.iterator->value.first = nativeImage->platformImage(); else result = false; However, this means using RefPtr<NativeImage> instead of auto; I have doubts whether this frowned upon. Advice? (In reply to Jim Mason from comment #7) > Thanks for the feedback. > > In setIconForPageURL, I am tempted to simplify this: > > auto image = BitmapImage::create(); > if (image->setData(SharedBuffer::create(iconData, iconDataSize), true) < > EncodedDataStatus::SizeAvailable) > result = false; > else if (auto nativeImage = image->nativeImageForCurrentFrame()) > addResult.iterator->value.first = nativeImage->platformImage(); > else > result = false; > > to something like this: > > RefPtr<NativeImage> nativeImage; > auto image = BitmapImage::create(); > if (image->setData(SharedBuffer::create(iconData, iconDataSize), true) > >= EncodedDataStatus::SizeAvailable && > (nativeImage = image->nativeImageForCurrentFrame())) > addResult.iterator->value.first = nativeImage->platformImage(); > else > result = false; > > However, this means using RefPtr<NativeImage> instead of auto; I have doubts > whether this frowned upon. Advice? There are similar bits of code all around WebKit, this is fine 👌️ Created attachment 427648 [details]
Patch
Committed r276954 (237287@main): <https://commits.webkit.org/237287@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 427648 [details]. Fix verified in r276954. *** Bug 225348 has been marked as a duplicate of this bug. *** |