|
This will examine the algorithm known as "the end" from the HTML
specification, which executes when parsing HTML markup has completed,
and it's potential to observably run script or change certain
attributes.
This currently executes in our engine when parsing HTML received from
the internet during navigation, using document.{open,write,close},
setting the innerHTML attribute or using DOMParser. The latter two are
only possible by executing script.
This has been causing some issues in our engine, which will be shown
later, so we are considering removing the call to "the end" for these
two cases.
Spoiler: the implications of running "the end" for DOMParser will be
considered in the future. It is the only script-created HTML/XML parser
remaining after this commit that uses "the end", including it's XML
variant implemented as XMLDocumentBuilder::document_end().
This will only focus on setting the innerHTML attribute, which falls
under "HTML fragment parsing", which starts here in the specification:
https://html.spec.whatwg.org/multipage/parsing.html#parsing-html-fragments
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/HTML/Parser/HTMLParser.cpp#L3491
While you may notice our HTMLParser::parse_html_fragment returns `void`
and assume this means no scripts are executed because of our use of
`WebIDL::ExceptionOr<T>` and `JS::ThrowCompletionOr<T>`, note that
dispatched events will execute arbitrary script via a callback, catch
any exceptions, report them and not propagate them. This means that
while a function does not return an exception type, it can still
potentially execute script.
A breakdown of the steps of "the end" in the context of HTML fragment
parsing and its observability follows:
https://html.spec.whatwg.org/multipage/parsing.html#the-end
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/HTML/Parser/HTMLParser.cpp#L221
1. No-op, as we don't currently have speculative HTML parsing. Even if
we did, we would instantly return after stopping the speculative
HTML parser anyway.
2. No-op, document.{open,write,close} are not accessible from the
temporary document.
3. No-op, document.readyState, window.navigation.timing and the
readystatechange event are not accessible from the created temporary
document.
4. This is presumably done so that reentrant invocation of the HTML
parser from document.{write,close} during the firing of the events
after step 4 ends up parsing from a clean state. This is a no-op, as
the events after step 4 do not fire and are not accessible.
5. No-op, we set HTMLScriptElement::m_already_started to true when
creating it whilst parsing an HTML fragment, which causes
HTMLScriptElement::prepare_script to instantly bail, meaning
`scripts_to_execute_when_parsing_has_finished` is always empty.
6. No-op, tasks are considered not runnable when the document does not
have a browsing context, which is always the case in fragment
parsing. Additionally, window.navigation.timing and the
DOMContentLoaded event aren't reachable from the temporary document.
7. Almost a no-op, `scripts_to_execute_as_soon_as_possible` is always
empty for the same reason as step 4. However, this step uses an
unconditional `spin_until` call, which _is_ observable and causes
one of the alluded to issues, which will be talked about later.
8. No-op, as delaying the load event has no purpose in this case, as
the task in step 9 will set the current document readiness to
"complete" and then return immediately after, as the temporary
document has no browsing context, skipping the Window load event.
However, this step causes another alluded to issue, which will be
talked about later.
9. No-op, for the same reason as step 6. Additionally,
document.readyState is not accessible from the temporary document
and the temporary document has no browsing context, so navigation
timing, the Window load event, the pageshow event, the Document load
event and the `<iframe>` load steps are not executed at all.
10. No-op, as this flag is only set from window.print(), which is not
accessible for this document.
11. No-op, as the temporary document is not accessible from anything
else and will be immediately destroyed after HTML fragment parsing.
Additionally, browsing context containers (`<iframe>`, `<frame>` and
`<object>`) cannot run in documents with no browsing context:
- `<iframe>` and `<frame>` use "create a new child navigable":
https://html.spec.whatwg.org/multipage/document-sequences.html#create-a-new-child-navigable
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/HTML/BrowsingContextContainer.cpp#L43-L45
> 2. Let group be element's node document's browsing context's
top-level browsing context's group.
This requires the element's node document's browsing context to be
non-null, but it is always null with the temporary document created for
HTML fragment parsing.
This is protected against here for `<iframe>`:
https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-iframe-element:the-iframe-element-6
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/HTML/HTMLIFrameElement.cpp#L45
> When an iframe element element is inserted into a document whose
browsing context is non-null, the user agent must run these steps:
1. Create a new child navigable for element.
This is currently not protected against for `<frame>` in the
specification:
https://html.spec.whatwg.org/multipage/obsolete.html#active-frame-element
> A frame element is said to be an active frame element when it is in a
document.
> When a frame element element is created as an active frame element,
or becomes an active frame element after not having been one, the
user agent must run these steps:
> 1. Create a new child navigable for element.
However, since this would cause a null dereference, this is actually a
specification issue. See: https://github.com/whatwg/html/issues/9136
- `<object>` uses "queue an element task" and has a browsing context
null check.
https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-object-element:queue-an-element-task
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/HTML/HTMLObjectElement.cpp#L58
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/HTML/HTMLObjectElement.cpp#L105
> ...the user agent must queue an element task on the DOM manipulation
task source given the object element to run the following steps to
(re)determine what the object element represents.
As established above, tasks are not runnable in documents with null
browsing contexts. However, for avoidance of doubt, it checks if the
document's browsing context is null, and if so, it falls back to
representing the element's children and gets rid of any child navigable
the `<object>` element may have.
> 2. If the element has an ancestor media element, or has an ancestor
object element that is not showing its fallback content, or if the
element is not in a document whose browsing context is non-null,
or if the element's node document is not fully active, or if the
element is still in the stack of open elements of an HTML parser
or XML parser, or if the element is not being rendered, then jump
to the step below labeled fallback.
> 4. Fallback: The object element represents the element's children.
This is the element's fallback content. Destroy a child navigable
given the element.
This check also protects against an `<object>` element being adopted
from a document which has a browsing context to one that doesn't during
the time between the element task being queued and then executed.
This means a browsing context container cannot be ran, meaning browsing
context containers cannot access their parent document and access the
properties and events mentioned in steps 1-11 above, or use
document.{open,write,close} on the parent document.
Another potential avenue of running script via HTML fragment parsing
is via custom elements being in the markup, which need to be
synchronously upgraded. For example:
```
<custom-element></custom-element>
```
However, this is already protected against in the spec:
https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/HTML/Parser/HTMLParser.cpp#L643
> 7. If definition is non-null and the parser was not created as part
of the HTML fragment parsing algorithm, then let will execute
script be true. Otherwise, let it be false.
It is protected against overall by disabling custom elements via
returning `null` for all custom element definition lookups if the
document has no browsing context, which is the case for the temporary
document:
https://html.spec.whatwg.org/multipage/custom-elements.html#look-up-a-custom-element-definition
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/DOM/Document.cpp#L2106-L2108
> 2. If document's browsing context is null, return null.
This is because the document doesn't have an associated Window, meaning
there will be no associated CustomElementRegistry object.
After running the HTML fragment parser, all of the child nodes are
removed the temporary document and then adopted into the context
element's node document. Skipping the `pre_remove` steps as they are
not relevant in this case, let's first examine Node::remove()'s
potential to execute script, then examine Document::adopt_node() after.
https://dom.spec.whatwg.org/#concept-node-remove
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/DOM/Node.cpp#L534
1-7. Does not run any script, it just keeps a copy of some data that
will be needed later in the algorithm and directly modifies live
range attributes. However, since this relies on Range objects
containing the temporary document, the Range steps are no-ops.
8. Though this uses the temporary document, it does not contain any
NodeIterator objects as no script should have run, thus this
callback will not be entered. Even if the document _did_ have
associated NodeIterators, NodeIterator::run_pre_removing_steps does
not execute any script.
9-11. Does not run any script, it just keeps a copy of some data that
will be needed later in the algorithm and performs direct tree
mutation to remove the node from the node tree.
12-14. "assign slottables" and step 13 queue mutation observer
microtasks via "signal a slot change". However, since this is
done _after_ running "the end", the "spin the event loop" steps
in that algorithm does not affect this. Remember that queued
microtasks due not execute during this algorithm for the next
few steps.
Sidenote:
Microtasks are supposed to be executed when the JavaScript execution
context stack is empty. Since HTMLParser::parse_html_fragment is only
called from script, the stack will never be empty whilst it is running,
so microtasks will not run until some time after we exit this function.
15. This could potentially run script, let's have a look at the
removal steps we currently have implemented in our engine:
- HTMLIFrameElement::removed_from()
https://html.spec.whatwg.org/multipage/iframe-embed-object.html#the-iframe-element:the-iframe-element-7
https://github.com/SerenityOS/serenity/blob/44cf92616e59bda951b67cdae78a6361bdd76f7a/Userland/Libraries/LibWeb/HTML/HTMLIFrameElement.cpp#L102
Since browsing context containers cannot create child browsing
contexts (as shown above), this code will do nothing. This will also
hold true when we implement HTMLFrameElement::removed_from() in the
future.
- FormAssociatedElement::removed_from()
https://github.com/SerenityOS/serenity/blob/44cf92616e59bda951b67cdae78a6361bdd76f7a/Userland/Libraries/LibWeb/HTML/FormAssociatedElement.h#L36
This calls `form_node_was_removed` which can then potentially call
`reset_form_owner`. However, `reset_form_owner` only does tree
traversal to find the appropriate form owner and does not execute
any script. After calling `form_node_was_removed` it then calls
`form_associated_element_was_removed`, which is a virtual function
that no one currently overrides, meaning no script is executed.
- HTMLBaseElement::removed_from()
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/HTML/HTMLBaseElement.cpp#L45
This will call `Document::update_base_element` to do tree traversal
to find out the new first `<base>` element with an href attribute and
thus does not execute any script.
- HTMLStyleElement::removed_from()
https://html.spec.whatwg.org/multipage/semantics.html#update-a-style-block
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/HTML/HTMLStyleElement.cpp#L49
This will call `update_a_style_block`, which will parse the `<style>`
element's text content as CSS and create a style sheet from it. This
does not execute any script.
In summary, step 15 does not currently execute any script and ideally
shouldn't in the future when we implement more `removed_from` steps.
16. Does not run any script, just saves a copy of a variable.
17. Queues a "disconnectedCallback" custom elements callback. This will
execute script in the future, but not here.
18. Performs step 15 and 17 in combination for each of the node's
descendants. This will not execute any script.
19. Does not run any script, it performs a requirement of mutation
observers by adding certain things to a list.
20. Does not execute any script, as mutation observer callbacks are
done via microtasks.
21. This will not execute script, as the parent is always the temporary
document in HTML fragment parsing. There is no Document children
changed steps, so this step is a no-op.
We then do layout invalidation which is our own addition, but this also
does not execute any script.
In short, removing a node does not execute any script. It could execute
script in the future, but since this is done by tasks, it will not
execute until we are outside of HTMLParser::parse_html_fragment.
Let's look at adopting a node:
https://dom.spec.whatwg.org/#concept-node-adopt
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/DOM/Document.cpp#L1414
1. Does not run script, it just keeps a reference to the temporary
document.
2. No-op, we removed the node above.
3.1. Does not execute script, it simply updates all descendants of
the removed node to be in the context element's node document.
3.2. Does not execute script, see node removal step 17.
3.3. This could potentially execute script, let's have a look at the
adopting steps we have implemented in our engine:
- HTMLTemplateElement::adopted_from()
https://html.spec.whatwg.org/multipage/scripting.html#the-template-element:concept-node-adopt-ext
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/HTML/HTMLTemplateElement.cpp#L38
This simply adopts the `<template>` element's DocumentFragment node
into its inert document. This does not execute any script.
We then have our own addition of adopting NodeIterators over to the
context element's document, but this does not execute any script.
In short, adopting a node does not execute any script.
After adopting the nodes to the context element's document, HTML
fragment parsing is complete and the temporary document is no longer
accessible at all.
Document and element event handlers are also not accessible, even if
the event bubbles. This is simply because the temporary document is not
accessible, so tree traversal, IDL event handler attributes and
EventTarget#addEventListener are not accessible, on the document or any
descendants. Document is also not an Element, so element event handler
attributes do not apply.
In summary, this establishes that HTML fragment parsers should not run
any user script or internal C++ code that relies on things set up by
"the end". This means that the attributes set up and events fired by
"the end" are not observable in this case. This may have not explored
every single possible avenue, but the general assertion should still
hold. However, this assertion is violated by "the end" containing two
unconditional "spin the event loop" invocations and causes issues with
live web content, so we seek to avoid them.
As WebKit, Blink and Gecko have been able to get away with doing fast
path optimizations for HTML fragment parsing which don't setup
navigation timing, run events, etc. it is presumed we are able to get
away with not running "the end" for HTML fragment parsing as well.
WebKit: https://github.com/WebKit/WebKit/blob/c69be377e17c2977681fef9113d13d91b62d1ee4/Source/WebCore/dom/DocumentFragment.cpp#L90-L98
Blink: https://github.com/chromium/chromium/blob/15444426f98a99830338697cce54e686e988815c/third_party/blink/renderer/core/editing/serializers/serialization.cc#L681-L702
Gecko: https://github.com/mozilla/gecko-dev/blob/6fc2f6d5335fb6f70f780b5fea5ed77b0719c3b5/dom/base/FragmentOrElement.cpp#L1991-L2002
Removing the call to "the end" fixes at least a couple of issues:
- Inserting `<img>` elements via innerHTML causes us to spin forever.
This regressed in https://github.com/SerenityOS/serenity/commit/2413de7e10efda16f6f376b42b204279d7a25906
This is because `m_load_event_delayer.clear()` is performed inside an
element task callback. Because of the reasons stated above, this will
never execute. This caused us to spin forever on step 8 of "the end",
which is delaying the load event.
This affected Google Docs and Google Maps, never allowing them to
progress after performing this action. I have also seen it cause a
Scorecard Research `<img>` beacon in a `<noscript>` element inserted
via innerHTML to spin forever. This presumably affects many more
sites as well.
Given that the Window load event is not fired for HTML fragment
parsers, spinning the event loop to delay the load event does not
change anything, meaning this step can be skipped entirely.
- Microtask timing is messed up by the unconditional `spin_until`s on
steps 7 and 8.
"Spin the event loop" causes an unconditional microtask checkpoint:
https://html.spec.whatwg.org/multipage/webappapis.html#spin-the-event-loop
https://github.com/SerenityOS/serenity/blob/44dd8247647474df95137452b3c9cad9b83326be/Userland/Libraries/LibWeb/HTML/EventLoop/EventLoop.cpp#L54
> 3. Let old stack be a copy of the JavaScript execution context
stack.
> 4. Empty the JavaScript execution context stack.
> 5. Perform a microtask checkpoint.
> 6.2.1. Replace the JavaScript execution context stack with old
stack.
This broke YouTube with the introduction of custom elements, as
custom elements use microtasks to upgrade elements and call
callbacks. See https://github.com/whatwg/html/issues/8646 for a full
example reduced from YouTube's JavaScript.
Another potential fix for this issue is to remove the above steps
from "spin the event loop". However, since we have another issue with
the use of "spin the event loop", it would be best to just avoid
both calls to it.
Considering all of the above, removing the call to "the end" is the way
forward for HTML fragment parsing, as all of it should be a no-op.
This is done by not simply returning from "the end" if the HTML parser
was created for HTML fragment parsing.
The end.
|
