Skip to content

Use Variant instead of raw pointer union in IndexValueEntry#62318

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
szewai:eng/Use-Variant-instead-of-raw-pointer-union-in-IndexValueEntry
Apr 13, 2026
Merged

Use Variant instead of raw pointer union in IndexValueEntry#62318
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
szewai:eng/Use-Variant-instead-of-raw-pointer-union-in-IndexValueEntry

Conversation

@szewai
Copy link
Copy Markdown
Contributor

@szewai szewai commented Apr 8, 2026 •
edited by webkit-early-warning-system
Loading

86df899

Use Variant instead of raw pointer union in IndexValueEntry
https://bugs.webkit.org/show_bug.cgi?id=311777
rdar://174363490

Reviewed by Ryosuke Niwa.

Replace the union of raw IDBKeyData*/IDBKeyDataSet* with Variant<std::optional<IDBKeyData>, IDBKeyDataSet>, which
manages lifetime automatically and provides type safety via WTF::switchOn. This eliminates the manual destructor, the
m_unique bool, and all heap allocations for the unique-index case.

Remove some NODELETE annotations since new implementation gets "it contains code that could destruct an object" SaferCPP
error because of them.

* Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp:
(WebCore::IDBServer::constructEmptyKeys):
(WebCore::IDBServer::IndexValueEntry::IndexValueEntry):
(WebCore::IDBServer::IndexValueEntry::addKey):
(WebCore::IDBServer::IndexValueEntry::removeKey):
(WebCore::IDBServer::IndexValueEntry::contains):
(WebCore::IDBServer::IndexValueEntry::getCount const):
(WebCore::IDBServer::IndexValueEntry::Iterator::Iterator):
(WebCore::IDBServer::IndexValueEntry::Iterator::key const):
(WebCore::IDBServer::IndexValueEntry::Iterator::isValid const):
(WebCore::IDBServer::IndexValueEntry::Iterator::operator++):
(WebCore::IDBServer::IndexValueEntry::keys const):
(WebCore::IDBServer::IndexValueEntry::~IndexValueEntry): Deleted.
* Source/WebCore/Modules/indexeddb/server/IndexValueEntry.h:
(WebCore::IDBServer::IndexValueEntry::isUnique const):
(WebCore::IDBServer::IndexValueEntry::unique const): Deleted.
* Source/WebCore/Modules/indexeddb/server/IndexValueStore.h:

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

288ba81

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 win ⏳ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2   🧪 win-tests ⏳ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ⏳ 🛠 vision-apple
  🧪 ios-wk2-wpt   🧪 api-mac-debug ✅ 🛠 gtk3-libwebrtc
✅ 🧪 api-ios   🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 ios-safer-cpp ✅ 🧪 mac-wk2   🧪 gtk-wk2
  🛠 vision   🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress   🛠 playstation
  🧪 vision-wk2   🧪 mac-intel-wk2
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
  🛠 tv-sim
  🛠 watch
✅ 🛠 watch-sim

@szewai szewai self-assigned this Apr 8, 2026
@szewai szewai added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Apr 8, 2026
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Apr 8, 2026 •
edited
Loading

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

Details

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

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 9, 2026
@szewai szewai removed the merging-blocked Applied to prevent a change from being merged label Apr 9, 2026
@szewai szewai force-pushed the eng/Use-Variant-instead-of-raw-pointer-union-in-IndexValueEntry branch from cff172e to 5a41d65 Compare April 9, 2026 19:57
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Apr 9, 2026 •
edited
Loading

EWS run on previous version of this PR (hash 5a41d65)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 win ✅ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2   🧪 win-tests ✅ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ❌ 🛠 vision-apple
✅ 🧪 ios-wk2-wpt   🧪 api-mac-debug ✅ 🛠 gtk3-libwebrtc
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🛠 gtk
  🛠 ios-safer-cpp ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress   🛠 playstation
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv ❌ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@szewai szewai marked this pull request as ready for review April 9, 2026 21:20
@webkit-ews-buildbot
Copy link
Copy Markdown
Collaborator

webkit-ews-buildbot commented Apr 9, 2026

macOS Safer C++ Build #91808 (5a41d65)

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

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 9, 2026
@szewai szewai removed the merging-blocked Applied to prevent a change from being merged label Apr 10, 2026
@szewai szewai force-pushed the eng/Use-Variant-instead-of-raw-pointer-union-in-IndexValueEntry branch from 5a41d65 to 9c7cdd4 Compare April 10, 2026 00:21
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Apr 10, 2026 •
edited
Loading

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

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 win ✅ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2   🧪 win-tests ✅ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac loading-orange 🧪 api-wpe ✅ 🛠 vision-apple
✅ 🧪 ios-wk2-wpt ✅ 🧪 api-mac-debug ✅ 🛠 gtk3-libwebrtc
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🛠 gtk
❌ 🛠 ios-safer-cpp ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress   🛠 playstation
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv ❌ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot
Copy link
Copy Markdown
Collaborator

webkit-ews-buildbot commented Apr 10, 2026

macOS Safer C++ Build #91891 (9c7cdd4)

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

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 10, 2026
rniwa
rniwa reviewed Apr 10, 2026
View reviewed changes
Comment thread Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp Outdated
if (m_unique) {
delete m_key;
m_key = new IDBKeyData(key);
if (unique()) {
Copy link
Copy Markdown
Member

@rniwa rniwa Apr 10, 2026

Choose a reason for hiding this comment

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

It seems like we can use WTF::visit here?

Copy link
Copy Markdown
Contributor Author

@szewai szewai Apr 10, 2026

Choose a reason for hiding this comment

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

Is your suggestion about resolving the SaferCPP errors?

My last attempt used WTF::switchOn (5a41d65) and I got the same SaferCPP errors, so I switched to use std::get to avoid destruction of lambda. But it seems still not work; and I am not sure how to fix the SaferCPP errors.

Copy link
Copy Markdown
Member

@rniwa rniwa Apr 10, 2026

Choose a reason for hiding this comment

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

The bot is complaining about NODELETE annotation. You could simply remove that from the member function declarations.

Copy link
Copy Markdown
Contributor Author

@szewai szewai Apr 10, 2026

Choose a reason for hiding this comment

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

Ok didn't know we can delete annotation; that seems convenient. Though I might get other errors for removing the annotation.

rniwa
rniwa reviewed Apr 10, 2026
View reviewed changes
Comment thread Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp Outdated
rniwa
rniwa reviewed Apr 10, 2026
View reviewed changes
Comment thread Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp Outdated
rniwa
rniwa reviewed Apr 10, 2026
View reviewed changes
Comment thread Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp Outdated
rniwa
rniwa reviewed Apr 10, 2026
View reviewed changes
Comment thread Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp Outdated
rniwa
rniwa reviewed Apr 10, 2026
View reviewed changes
Comment thread Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp Outdated
rniwa
rniwa reviewed Apr 10, 2026
View reviewed changes
Comment thread Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp Outdated
rniwa
rniwa reviewed Apr 10, 2026
View reviewed changes
Comment thread Source/WebCore/Modules/indexeddb/server/IndexValueEntry.h Outdated
@webkit-ews-buildbot
Copy link
Copy Markdown
Collaborator

webkit-ews-buildbot commented Apr 10, 2026

iOS Safer C++ Build #10774 (9c7cdd4)

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

@szewai szewai removed the merging-blocked Applied to prevent a change from being merged label Apr 10, 2026
@szewai szewai force-pushed the eng/Use-Variant-instead-of-raw-pointer-union-in-IndexValueEntry branch from 9c7cdd4 to 5d30bf5 Compare April 10, 2026 17:10
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Apr 10, 2026 •
edited
Loading

EWS run on previous version of this PR (hash 5d30bf5)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win ✅ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ❌ 🧪 win-tests ✅ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ✅ 🛠 vision-apple
✅ 🧪 ios-wk2-wpt ✅ 🧪 api-mac-debug ✅ 🛠 gtk3-libwebrtc
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🛠 gtk
❌ 🛠 ios-safer-cpp ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🛠 playstation
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv ❌ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot
Copy link
Copy Markdown
Collaborator

webkit-ews-buildbot commented Apr 10, 2026

macOS Safer C++ Build #92083 (5d30bf5)

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

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 10, 2026
@webkit-ews-buildbot
Copy link
Copy Markdown
Collaborator

webkit-ews-buildbot commented Apr 11, 2026

iOS Safer C++ Build #10976 (5d30bf5)

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

@szewai szewai removed the merging-blocked Applied to prevent a change from being merged label Apr 13, 2026
@szewai szewai force-pushed the eng/Use-Variant-instead-of-raw-pointer-union-in-IndexValueEntry branch from 5d30bf5 to 288ba81 Compare April 13, 2026 17:23
@webkit-early-warning-system
Copy link
Copy Markdown
Collaborator

webkit-early-warning-system commented Apr 13, 2026 •
edited
Loading

EWS run on current version of this PR (hash 288ba81)

Details

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows Apple Internal
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 win ⏳ 🛠 ios-apple
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2   🧪 win-tests ⏳ 🛠 mac-apple
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe ⏳ 🛠 vision-apple
  🧪 ios-wk2-wpt   🧪 api-mac-debug ✅ 🛠 gtk3-libwebrtc
✅ 🧪 api-ios   🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 ios-safer-cpp ✅ 🧪 mac-wk2   🧪 gtk-wk2
  🛠 vision   🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress   🛠 playstation
  🧪 vision-wk2   🧪 mac-intel-wk2
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
  🛠 tv-sim
  🛠 watch
✅ 🛠 watch-sim

@szewai szewai added the merge-queue Applied to send a pull request to merge-queue label Apr 13, 2026
@szewai
Use Variant instead of raw pointer union in IndexValueEntry
86df899 
https://bugs.webkit.org/show_bug.cgi?id=311777
rdar://174363490

Reviewed by Ryosuke Niwa.

Replace the union of raw IDBKeyData*/IDBKeyDataSet* with Variant<std::optional<IDBKeyData>, IDBKeyDataSet>, which
manages lifetime automatically and provides type safety via WTF::switchOn. This eliminates the manual destructor, the
m_unique bool, and all heap allocations for the unique-index case.

Remove some NODELETE annotations since new implementation gets "it contains code that could destruct an object" SaferCPP
error because of them.

* Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp:
(WebCore::IDBServer::constructEmptyKeys):
(WebCore::IDBServer::IndexValueEntry::IndexValueEntry):
(WebCore::IDBServer::IndexValueEntry::addKey):
(WebCore::IDBServer::IndexValueEntry::removeKey):
(WebCore::IDBServer::IndexValueEntry::contains):
(WebCore::IDBServer::IndexValueEntry::getCount const):
(WebCore::IDBServer::IndexValueEntry::Iterator::Iterator):
(WebCore::IDBServer::IndexValueEntry::Iterator::key const):
(WebCore::IDBServer::IndexValueEntry::Iterator::isValid const):
(WebCore::IDBServer::IndexValueEntry::Iterator::operator++):
(WebCore::IDBServer::IndexValueEntry::keys const):
(WebCore::IDBServer::IndexValueEntry::~IndexValueEntry): Deleted.
* Source/WebCore/Modules/indexeddb/server/IndexValueEntry.h:
(WebCore::IDBServer::IndexValueEntry::isUnique const):
(WebCore::IDBServer::IndexValueEntry::unique const): Deleted.
* Source/WebCore/Modules/indexeddb/server/IndexValueStore.h:

Canonical link: https://commits.webkit.org/311131@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Use-Variant-instead-of-raw-pointer-union-in-IndexValueEntry branch from 288ba81 to 86df899 Compare April 13, 2026 19:15
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

webkit-commit-queue commented Apr 13, 2026

Committed 311131@main (86df899): https://commits.webkit.org/311131@main

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

@webkit-commit-queue webkit-commit-queue merged commit 86df899 into WebKit:main Apr 13, 2026
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rniwa rniwa rniwa approved these changes

@beidson beidson Awaiting requested review from beidson

@pvollan pvollan Awaiting requested review from pvollan

@RupinMittal RupinMittal Awaiting requested review from RupinMittal

Assignees

@szewai szewai

Labels

New Bugs Unclassified bugs are placed in this component until the correct component can be determined.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@szewai @webkit-early-warning-system @webkit-ews-buildbot @webkit-commit-queue @rniwa