From f52ede23aa5399120d3271605cc7ea0cd28baa18 Mon Sep 17 00:00:00 2001
From: Luke Wilde <lukew@serenityos.org>
Date: Tue, 11 Apr 2023 19:19:30 +0100
Subject: LibWeb: Return from "the end" during HTML fragment parsing

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.
---
 .../Libraries/LibWeb/HTML/Parser/HTMLParser.cpp    | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

(limited to 'Userland/Libraries/LibWeb')

diff --git a/Userland/Libraries/LibWeb/HTML/Parser/HTMLParser.cpp b/Userland/Libraries/LibWeb/HTML/Parser/HTMLParser.cpp
index fccbd1f256..c4b5b6eba6 100644
--- a/Userland/Libraries/LibWeb/HTML/Parser/HTMLParser.cpp
+++ b/Userland/Libraries/LibWeb/HTML/Parser/HTMLParser.cpp
@@ -222,6 +222,28 @@ void HTMLParser::the_end()
 {
     // Once the user agent stops parsing the document, the user agent must run the following steps:
 
+    // The entirety of "the end" should be a no-op for HTML fragment parsers, because:
+    // - the temporary document is not accessible, making the DOMContentLoaded event and "ready for post load tasks" do
+    //   nothing, making the parser not re-entrant from document.{open,write,close} and document.readyState inaccessible
+    // - there is no Window associated with it and no associated browsing context with the temporary document (meaning
+    //   the Window load event is skipped and making the load timing info inaccessible)
+    // - scripts are not able to be prepared, meaning the script queues are empty.
+    // However, the unconditional "spin the event loop" invocations cause two issues:
+    // - Microtask timing is changed, as "spin the event loop" performs an unconditional microtask checkpoint, causing
+    //   things to happen out of order. For example, YouTube sets the innerHTML of a <template> element in the constructor
+    //   of the ytd-app custom element _before_ setting up class attributes. Since custom elements use microtasks to run
+    //   callbacks, this causes custom element callbacks that rely on attributes setup by the constructor to run before
+    //   the attributes are set up, causing unhandled exceptions.
+    // - Load event delaying can spin forever, e.g. if the fragment contains an <img> element which stops delaying the
+    //   load event from an element task. Since tasks are not considered runnable if they're from a document with no
+    //   browsing context (i.e. the temporary document made for innerHTML), the <img> element will forever delay the load
+    //   event and cause an infinite loop.
+    // We can avoid these issues and also avoid doing unnecessary work by simply skipping "the end" for HTML fragment
+    // parsers.
+    // See the message of the commit that added this for more details.
+    if (m_parsing_fragment)
+        return;
+
     // FIXME: 1. If the active speculative HTML parser is not null, then stop the speculative HTML parser and return.
 
     // 2. Set the insertion point to undefined.
-- 
cgit v1.2.3