1925468 - Implement Trusted Type support for setAttribute/setAttributeNS

Status: RESOLVED FIXED

(Core :: DOM: Security, task)

Description

[Frédéric Wang (:fredw)]

Attachment: set-event-handler-attribute-with-trusted-script-policy.html

See attached testcase.

IIUC this corresponds to this PR to the DOM spec for setAttribute/setAttributeNS as well as the following sections from the trusted type spec.

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

This is also at least making the following tests fail:

trusted-types/trusted-types-event-handlers.html (see testing/web-platform/meta/trusted-types/trusted-types-event-handlers.html.ini)
trusted-types/TrustedTypePolicyFactory-metadata.tentative.html (failure shows up after implementing getAttributeType in bug 1917783)

Comment 1

[Mirko Brodesser (:mbrodesser) -- not reading bugmails currently]

https://searchfox.org/mozilla-central/rev/a547d790fa7facb8d0addb9197797dc52b31e8f2/testing/web-platform/tests/trusted-types/block-string-assignment-to-Element-setAttribute.html contains tests too.

Comment 2

[Frédéric Wang (:fredw)]

Updating title, event handler content attribute are only one case.

In general, setAttribute/setAttributeNS should handle all attributes mentioned on https://w3c.github.io/trusted-types/dist/spec/#abstract-opdef-get-trusted-type-data-for-attribute

Similar logic is implemented in TrustedTypePolicyFactory::GetAttributeType and we should make sure to share code as much as possible.

Comment 3

[Frédéric Wang (:fredw)]

Attachment: Bug 1925468 - Factor out "Get Trusted Type Data For Attribute". r=smaug

This patch introduces TrustedTypeUtils::GetTrustedTypeDataForAttribute
to implement the corresponding algorithm from the spec:
https://w3c.github.io/trusted-types/dist/spec/#get-trusted-type-data-for-attribute

It is currently only used by TrustedTypePolicyFactory::GetAttributeType
but "Get Trusted Types-compliant attribute value" will use it to when
we implement trusted types for setAttribute/setAttributeNS.
https://w3c.github.io/trusted-types/dist/spec/#validate-attribute-mutation

Comment 4

[Frédéric Wang (:fredw)]

Attachment: Bug 1925468 - Implement Trusted Type support for setAttribute/setAttributeNS. r=smaug

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

Comment 5

[Frédéric Wang (:fredw)]

Attachment: Bug 1925468 - Extend and improve modify-attributes-in-callback.html. r=smaug

• Use cleanup functions to guarantee the state of the DOM tree before
  each test.
• Make more explicit why the length of the div's attribute list is
  expected to be 2.
• Add 2 tests for https://g-issues.chromium.org/issues/333739948
  using setAttributeNS instead of setAttribute.

Comment 6

[Frédéric Wang (:fredw)]

Attachment: setAttribute testcase for profiling

Attached is a basic performance test for profiling purpose:

• It sets attribute via setAttribute and setAttributeNS.
• These calls perform attaching a new attribute and then modifying its value.
• It tries attributes for which GetTrustedTypeDataForAttribute returns false (title) and true (onclick).
• It can test DOM mutation in either window.document or a "data" document.

Comment 7

[Frédéric Wang (:fredw)]

These are the profiling results for attachment 9437631 [details] with 0.01ms sampling interval:

• dom.security.trusted_types disabled: https://share.firefox.dev/4eAhAE2
• dom.security.trusted_types enabled: https://share.firefox.dev/48QGsX5
• dom.security.trusted_types enabled, data document: https://share.firefox.dev/48NCpL4
• dom.security.trusted_types enabled, data document, testcase tweaked to only set the title attribute: https://share.firefox.dev/4fse5AN

I believe things look as expected:

• The call to the "string" version of Element::SetAttribute from the "trusted type" version does not show up on the flame graph, thanks to the nsMutationGuard.

• For the two first profiles, the call to GetTrustedTypesCompliantAttributeValue does not show up either when filtering for SetAttribute. When filtering for GetTrustedTypesCompliantAttributeValue, we see that it exits early as expected ; we get no results when filtering for GetTrustedTypeDataForAttribute or GetTrustedTypesCompliantString.

• For the third profile, the call to GetTrustedTypesCompliantAttributeValue shows up on the flame graph when filtering for SetAttribute. Filtering for GetTrustedTypesCompliantAttributeValue, it is essentially spending time in GetTrustedTypeDataForAttribute (IsEventAttributeName doing a map lookup) and GetTrustedTypesCompliantString (retrieving the global window object). We get no results when filtering for GetScp or DoesSinkTypeRequireTrustedTypes, as previously tested for innerHTML.

• For the fourth profile, the call to GetTrustedTypesCompliantAttributeValue does not show up either when filtering for SetAttribute. When filtering for GetTrustedTypesCompliantAttributeValue, we see that it exits early after the GetTrustedTypeDataForAttribute call (IsEventAttributeName exits early without map lookup) ; we get no results when filtering for GetTrustedTypesCompliantString.

Comment 8

[Frédéric Wang (:fredw)]

Patch set at https://lando.services.mozilla.com/D227943 have been updated to use c++ templates and address review feedback.

New try job is available at : https://treeherder.mozilla.org/jobs?repo=try&revision=a2df508e2bd1f0c7c01f72dbce8fd9eb00d47df1
try perf submitted here: https://perf.compare/compare-results?baseRev=e9a124617383f6f84e987c5e91bbcab5b4033c83&newRev=91dc606f59b35aeda8987ed705a92c8d7ffd8f0e&baseRepo=try&newRepo=try&framework=13

Comment 9

[Frédéric Wang (:fredw)]

So https://perf.compare/subtests-compare-results?baseRev=e9a124617383f6f84e987c5e91bbcab5b4033c83&baseRepo=try&newRev=91dc606f59b35aeda8987ed705a92c8d7ffd8f0e&newRepo=try&framework=13&baseParentSignature=5131097&newParentSignature=5131097 does not report any regression.

Looking over each subtest, I guess the only significant difference is the 11% for Editor-TipTap/Highlight/Async, which seems due to 3 slow runs: https://treeherder.mozilla.org/perfherder/graphs?highlightedRevisions=e9a124617383&highlightedRevisions=91dc606f59b3&series=try%2C50f3328343643f6c9f3668f4c30ae2baf64165c5%2C1%2C13&timerange=86400

Comment 10

[Frédéric Wang (:fredw)]

@smaug: I'm setting the needinfo flag, so you can take a look whenever you have time:

1. Review the stack at https://lando.services.mozilla.com/D227943 ; they are all approved but would be good to take a look now that things moved to C++ templates.

2. Check profiling results (comment 7)

3. Check try perf results (comment 8 and comment 9)

I plan to land the patches one bug at a time, but won't do it until I get your feedback on these points.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I'm not seeing any difference in https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&originalRevision=e9a124617383f6f84e987c5e91bbcab5b4033c83&newProject=try&newRevision=91dc606f59b35aeda8987ed705a92c8d7ffd8f0e&framework=13&originalSignature=5131097&newSignature=5131097&page=1&replicates=1&showOnlyImportant=1

That is showing only "important changes".

(need to still use the older perfherder to get links to share)

So I think sp3 looks good.

Comment 7 shows that you have a11y enabled? That slows down things quite a bit.
But the setAttribute method itself looks reasonable. Thanks for testing.

Comment 12

[Frédéric Wang (:fredw)]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #11)

Comment 7 shows that you have a11y enabled? That slows down things quite a bit.

Thanks for checking. Mmh, I'm not sure what's happening with these mozilla::a11y::EventQueue events, I don't think I have a11y enabled.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

about:support should tell if a11y is enabled.

(but that is unrelated to this bug, though I guess the new code might show up in the profiles in a bit different ways if a11y wasn't enabled)

Comment 14

[Frédéric Wang (:fredw)]

Yes, it seems to be enabled. Setting accessibility.force_disabled=1 works around that, although I'm not sure this is supposed to be necessary. anyway, as you said that's unrelated to that bug.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0efe13efdebc
Factor out "Get Trusted Type Data For Attribute". r=smaug
https://hg.mozilla.org/integration/autoland/rev/37b01d6a0d0d
Extend and improve modify-attributes-in-callback.html. r=smaug
https://hg.mozilla.org/integration/autoland/rev/db598441827d
Implement Trusted Type support for setAttribute/setAttributeNS. r=smaug

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/49394 for changes under testing/web-platform/tests

Comment 17

[Serban Stanca [:SerbanS]]

Backed out for causing multiple failures.

• Backout link

---

• Push with failures - build bustages
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/dom/security/trusted-types/TrustedTypeUtils.cpp:50:1: error: control reaches end of non-void function [-Werror=return-type]

---

• Push with failures - wpt failures
• Failure Log
• Failure line: TEST-UNEXPECTED-PASS | /trusted-types/trusted-types-event-handlers.html | Event handler div.ontouchstart should be blocked. - expected FAIL

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 19

[Frédéric Wang (:fredw)]

(In reply to Serban Stanca [:SerbanS] from comment #17)

It seems some platforms believe there is reachable code after the switch() statements, and Android passes more tests.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0f1354e4993
Factor out "Get Trusted Type Data For Attribute". r=smaug
https://hg.mozilla.org/integration/autoland/rev/094b1d5348ab
Extend and improve modify-attributes-in-callback.html. r=smaug
https://hg.mozilla.org/integration/autoland/rev/eb723beab1df
Implement Trusted Type support for setAttribute/setAttributeNS. r=smaug

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 22

[amarc]

https://hg.mozilla.org/mozilla-central/rev/d0f1354e4993
https://hg.mozilla.org/mozilla-central/rev/094b1d5348ab
https://hg.mozilla.org/mozilla-central/rev/eb723beab1df