See
https://github.com/whatwg/dom/pull/1268
https://w3c.github.io/trusted-types/dist/spec/#validate-attribute-mutation
https://w3c.github.io/trusted-types/dist/spec/#get-trusted-type-data-for-attribute
Details
Details
Diff Detail
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| dom/security/trusted-types/TrustedTypeUtils.cpp | ||
|---|---|---|
| 494 | This looks weird. Use of std::pair should be limited basically to some generic data structures. | |
| dom/security/trusted-types/TrustedTypeUtils.h | ||
| 68 | Could we use some proper simple struct for this. std::pair really should be used in some generic data structures. .first and .second are meaningless to the reader. | |
| testing/web-platform/meta/trusted-types/modify-attributes-in-callback.html.ini | ||
| 5 ↗ | (On Diff #941082) | Why do these two tests fail? |
| dom/base/Element.cpp | ||
|---|---|---|
| 1691 | Should I open a follow-up bug to remove this here and in the old SetAttribute? | |
| dom/security/trusted-types/TrustedTypeUtils.cpp | ||
| 494 | no longer necessary now that the expected type is passed as a template parameter. | |
| dom/security/trusted-types/TrustedTypeUtils.h | ||
| 68 | no longer necessary now that the expected type is passed as a template parameter. | |
| dom/base/Element.cpp | ||
|---|---|---|
| 1691 | Removed. | |
| dom/security/trusted-types/TrustedTypeUtils.cpp | ||
| 476 | Ack. | |
| 478 | I rewrote the code as suggested, but still need to do some profiling. | |
| testing/web-platform/meta/trusted-types/modify-attributes-in-callback.html.ini | ||
| 5 ↗ | (On Diff #941082) | I haven't debugged yet, but it seems this is similar issue as in WebKit/Chromium: https://issues.chromium.org/issues/333739948 Basically the Trusted type callback is editing the attribute list while the attribute is set, so depending on the internal structure used to represent the attribute list, that may result in the wrong modification happening. Will try to investigate more Firefox's case. |
Comment Actions
Code analysis found 3 defects in diff 944086:
- 1 defect found by clang-format
- 2 defects found by clang-format (Mozlint)
WARNING: Found 3 defects (warning level) that can be dismissed.
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
- ./mach clang-format -p dom/base/Element.cpp
For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 944086.
Comment Actions
Code analysis found 3 defects in diff 944099:
- 1 defect found by clang-format
- 2 defects found by clang-format (Mozlint)
WARNING: Found 3 defects (warning level) that can be dismissed.
You can run this analysis locally with:
- ./mach clang-format -p dom/base/Element.cpp
- ./mach lint --warnings --outgoing
For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 944099.
| dom/base/Element.cpp | ||
|---|---|---|
| 1719 | This feels too slow, indeed, and this is very hot code. But we should be able to optimize this by using nsMutationGuard, and it's Mutated() method. Only take the slow code path if DOM was mutated. | |
| dom/base/Element.cpp | ||
|---|---|---|
| 1719 | OK I've done that change. | |
Comment Actions
r+, but could you do some profiling. With and without the pref enabled, set some random attributes and share profiles here? Just to ensure we aren't missing anything obvious.
And perhaps push also to try and run at least Speedometer3 on Win11
./mach try perf --rebuild 20 --show-all
and select speedometer3 shippable firefox win11
(don't select hw-ref, but other win11)
This should land only once we have some perf numbers because this is so performance critical
| dom/security/trusted-types/TrustedTypeUtils.cpp | ||
|---|---|---|
| 457 | aNewValue | |
| dom/security/trusted-types/TrustedTypeUtils.cpp | ||
|---|---|---|
| 478 | Will do some profiling & perf testing today. | |
| dom/security/trusted-types/TrustedTypeUtils.cpp | ||
|---|---|---|
| 478 | ||
| dom/base/Element.cpp | ||
|---|---|---|
| 1719 | It seems I didn't reply about the "benchmark". I think this code is executed when modifying the value of an already attached attribute, as it is done in https://bug1925468.bmoattachments.org/attachment.cgi?id=9437631 ; i didn't try without my mutation guard, but the profiles with that change looks good. I think the slow path would only be executed if the trusted type call back performed some DOM mutation. Probably this can be optimized further, but it seems good enough to me as that does not sound a very frequent case in practice (i.e. a callback sanitizing the input). | |
Comment Actions
OK, I've sent this there. Profiling & try job looks good. let's wait for the speedometer3 result:
Profiling: https://bugzilla.mozilla.org/show_bug.cgi?id=1925468#c7
Try job and performance: https://bugzilla.mozilla.org/show_bug.cgi?id=1925468#c8