1936014 - Trusted Types performance testing and improvement

Status: NEW

(Core :: DOM: Security, task)

Description

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

This is a meta bug to verify performance before shipping trusted types. This was done by two means:

1. "try perf", specifically Speedometer 3 on Windows, to check for any regression.
2. Profiling with high sampling rate some simple test cases with "hot injection sinks" to detect any unwanted function call or extra work. For innerHTML we used https://mozilla.pettay.fi/moztests/innerhtml_nonsimple_nostyle_test.xhtml and attachment 9434166 [details] ; for setAttribute we used attachment 9437631 [details].

Both looked ok last time we checked (bug 1909168 and bug 1925468), but we may need to try again later.

We also discussed other ideas for potential perf improvements, I'll elaborate later.

Comment 1

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

(In reply to Frédéric Wang (:fredw) from comment #0)

We also discussed other ideas for potential perf improvements, I'll elaborate later.

OK, so first, let's explain the current situation.

We have many injection sinks and each time we end up calling GetTrustedTypesCompliantString this adds some overhead compared to the legacy code (where we were just using the input string as is). For some (but maybe not all) of the injection sinks, this can be a potential performance regression to consider. For completeness, I'm writing the current list here (the last fours apply to workers):

• Element.innerHTML, ShadowRoot's innerHTML, Element's outerHTML
• Element.insertAdjacentHTML
• Element/ShadowRoot's setHTMLUnsafe()
• document.write()/document.writeln()
• Element's setAttribute/setAttributeNS
• Document.parseHTMLUnsafe()
• DOMParser's parseFromString()
• HTMLScriptElement's src/innerText/textContent/text IDL attributes
• Range's createContextualFragment()
• Document's execCommand
• eval / new Function
• WindowOrWorkerGlobalScope's setInterval() / setTimeout
• Worker/SharedWorker's constructor
• WorkerGlobalScope's importScripts()

In bug 1909168, we are basically caching a boolean on nsCSPPolicy / nsIContentSecurityPolicy / Document objects indicating whether they have an associated require-trusted-types-for directive, so we can determine whether a call to ProcessValueWithADefaultPolicy is actually needed. This saves a search in a list of policies or directives, and even avoids unnecessary pointer lookup (e.g. retrieving the CSP associated to a document).

GetTrustedTypesCompliantString is currently implemented that way:

• If TT pref is disabled, return early.
• If the global object is a document without the require-trusted-types-for directive, then return early.
• Minimize amount of potentially costly operations (pointer lookup, memory allocation...).

Note that if aNodeOrGlobalObject is a node and owner doc is a data document then we need extra pointer lookup in order to obtain the global object (this is why test cases mentioned in comment 0 consider this case).

Note that simple GetTrustedTypesCompliantString* variants are generally used, but for setAttribute(NS) and eval/Function, the call to GetTrustedTypesCompliantString is performed via GetTrustedTypesCompliantAttributeValue and AreArgumentsTrustedForEnsureCSPDoesNotBlockStringCompilation helpers, which also add overhead and requires similar optimization as described above.

Last, in order to accept different C++ types as arguments to the TrustedTypeUtils helpers, we considered whether to wrap them in a mozilla::Variant (which can add runtime overhead) or to rely on C++ templates (which can increase binary size). Again, since optimizing runtime performance is the goal here, we leant toward the latter.

Comment 2

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

(In reply to Frédéric Wang (:fredw) from comment #0)

We also discussed other ideas for potential perf improvements, I'll elaborate later.

Regarding the ideas for improvements, I believe we only have two of them for now:

1. data documents are considered less critical than non-data documents, and so keeping the extra pointer lookup was assumed to be okay. If that turned out not to be the case, then "inner window could have a list to all the documents which have it as the scope object. And whenever TT status changes, all the documents would be updated." (bug 1909168 comment 9)
2. Once we make TrustedTypeUtils functions work with WorkerGlobalScope (bug 1901492) then we might need similar optimizations as for Window global scope.

@smaug: do you remember anything else?

Comment 3

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

I don't think I have anything else for the case when TT isn't used.
We should still profile the cases when the page is using TT. There might be some low hanging fruits there, like having some fast paths for the case when Trusted<Foo> type is passed as an argument and no callback is needed. In case a callback is called, I assume just calling the callback is so slow that the TT implementation matters less. (JS<->C++ boundary is always a bit of a bottleneck). But some profiling would show if there are issues.

Comment 5

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

Some more notes for future reference:

Our code currently relies on an optimization that basically caches on a document/CSP list/policy a boolean indicating whether they contain any "require-trusted-types-for" directive.

This allows early returns in "Get Trusted Type Compliant String", even skipping the cost of pointer lookup for the CSP list when the doc's boolean says there is no such directive.

Now, because of there is only one sink group used in our code and accepted by the CSP parser, this is actually equivalent to the result of "Does sink type require trusted types?" so I'm simplifying things in D233505, making that algo O(1) and "Should sink type mismatch violation be blocked by Content Security Policy?" O(size of CSP list).

In D233506, I'm adding a similar boolean (actually I'm grouping the two booleans in a single enum) for whether there is an enforced "require-trusted-types-for" directive, which is really the result of "Should sink type mismatch violation be blocked by Content Security Policy?". Although we still need to browse the full list to decide to determine how many violations to report the result of the algo is obtained in O(1) and independently.

Finally in D233636, I'm exposing these booleans to the worker thread, so that in D233507 we can implement GetTrustedTypesCompliantString for workers (the booleans are read in the worker thread and the violation reports are done on the main thread).

The proposal trusted-types-eval at https://github.com/mozilla/standards-positions/issues/1032 relies on https://github.com/w3c/trusted-types/pull/518 which essentially condition whether "Does sink type require trusted types?" return the first or second boolean. That means the work I made to support workers will allow to preserve our optimization for bug 1940493.

Comment 6

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

Putting this for the record: https://phabricator.services.mozilla.com/D236107#inline-1319411 ; this is what I used to get rid of possible overhead with virtual override.