WebKit
WebKit copied to clipboard
WebKit
Implement GC handling for trusted types API
69378bcdefcff22417914a427298160ae5dddd75
Implement GC handling for trusted types API https://bugs.webkit.org/show_bug.cgi?id=268419 Reviewed by NOBODY (OOPS!). This patch implements visitAdditionalChildren for TrustedTypePolicy calling visitJSFunction on callbacks. TrustedTypePolicy is also an opaque root which is added inside of TrustedTypePolicyFactory visitAdditionalChildren. It also marks TrustedTypePolicyFactory as being reachable via scriptExecutionContext. * Source/WebCore/Sources.txt: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/bindings/js/JSTrustedTypePolicyCustom.cpp: Added. (WebCore::JSTrustedTypePolicy::visitAdditionalChildren): * Source/WebCore/bindings/js/JSTrustedTypePolicyFactoryCustom.cpp: Added. (WebCore::JSTrustedTypePolicyFactory::visitAdditionalChildren): * Source/WebCore/dom/TrustedTypePolicy.cpp: (WebCore::TrustedTypePolicy::TrustedTypePolicy): (WebCore::TrustedTypePolicy::createHTML): (WebCore::TrustedTypePolicy::createScript): (WebCore::TrustedTypePolicy::createScriptURL): (WebCore::root): * Source/WebCore/dom/TrustedTypePolicy.h: (WebCore::TrustedTypePolicy::options const): (WebCore::TrustedTypePolicy::WTF_RETURNS_LOCK): * Source/WebCore/dom/TrustedTypePolicy.idl: * Source/WebCore/dom/TrustedTypePolicyFactory.cpp: (WebCore::TrustedTypePolicyFactory::create): (WebCore::TrustedTypePolicyFactory::TrustedTypePolicyFactory): * Source/WebCore/dom/TrustedTypePolicyFactory.h: (WebCore::TrustedTypePolicyFactory::WTF_RETURNS_LOCK): * Source/WebCore/dom/TrustedTypePolicyFactory.idl: * Source/WebCore/dom/WindowOrWorkerGlobalScopeTrustedTypes.cpp: (WebCore::DOMWindowTrustedTypes::trustedTypes const): (WebCore::WorkerGlobalScopeTrustedTypes::trustedTypes const):
https://github.com/WebKit/WebKit/commit/69378bcdefcff22417914a427298160ae5dddd75
| 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 | ||||
| ✅ 🛠 watch-sim |
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/53d0cc42dfe5e0d925cf6058de4e9797d33fc8a6)
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/7caa71eda03bdad9152cd1aecb6b97302ad0db9a)
| 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 | ||||
| ✅ 🛠 watch-sim |
@rniwa is this the sort of changes you had in mind for GC handling of the trusted types API? It doesn't solve one of the memory issues that's come up so I think something is missing but not sure what exactly it is.
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/ec6f36ff0835dd5d8e6f0ac97fbd81d395e82048)
| 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 | ||||
| ✅ 🛠 watch-sim |
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/69378bcdefcff22417914a427298160ae5dddd75)
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/d9334411bc43851fdb4f9f713c19a04a3f5b9122)
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/01f92f0e6f2cdc7dde39c2d08e1d14d823e40195)
I think there's a problem with the TrustedTypePolicyOptions callbacks. They're by default strong references which makes them GC roots which is a memory leak footgun.
I would suggest since you're already implementing visitAdditionalChildren to make them weak refs (the internal IDL attribute is IsWeakCallback) and manage their memory via the owner class (TrustedTypesPolicy's visitAdditionalChildren).
I would suggest since you're already implementing visitAdditionalChildren to make them weak refs (the internal IDL attribute is IsWeakCallback) and manage their memory via the owner class (TrustedTypesPolicy's visitAdditionalChildren).
Okay I'll take a look into this and address it tomorrow
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/ec95fe8bfc2e2b56a076088ac519dfd667414fbe)
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/22d2d77e3cb6c721b8a0a57ffa742eb9b3a7121d)
The style linter wants me to name 3 variables the same thing and protector or protectedNullptr doesn't really feel great either way? I'm not sure what the appropriate thing to do in that case is.
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/cd7100e6883eba5f3d38ec62106afb3f62f153eb)
The style linter wants me to name 3 variables the same thing and protector or protectedNullptr doesn't really feel great either way? I'm not sure what the appropriate thing to do in that case is.
I think something like protected<type>Handler is reasonable.
It's this style rule: https://webkit.org/code-style-guidelines/#names-protectors
It's this style rule: https://webkit.org/code-style-guidelines/#names-protectors
Even if I change them to be called protectedCreateXXX it still complains that it's incorrectly named?
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/5758976219cc78bc87213d59febab33ff916e28d)
It's this style rule: https://webkit.org/code-style-guidelines/#names-protectors
Even if I change them to be called protectedCreateXXX it still complains that it's incorrectly named?
The checker is probably getting confused because we're accessing members of m_options. protectedCreateXXX is more correct IMO. protectedNullptr is just silly.
Ah it's getting confused because we're explicitly setting it to nullptr. If you just default construct the smart pointers (RefPtr<Type> protectedCreateWhatever;) it will probably pass.
EWS run on previous version of this PR (hash https://github.com/WebKit/WebKit/commit/eb43b12c20e92483faa30a865eaa993cde6e3bf1)
🧪 wpe-wk2
EWS run on current version of this PR (hash https://github.com/WebKit/WebKit/commit/c89c07202f8aecfb875b580d157fcd4d1900bcfc)
Committed 276974@main (3fe3c3955914): https://commits.webkit.org/276974@main
Reviewed commits have been landed. Closing PR #23999 and removing active labels.
