1475000 - document.open() doesn't work after window.stop()

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect, P3)

Description

[Timothy Gu [:TimothyGu]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36

Steps to reproduce:

Load the following HTML file in Firefox.

------8<------
<p>This should get erased.</p>
<script>
window.stop();
document.open();
</script>
------>8------


Actual results:

The `document.open()` call is ignored, and the page loads as usual.


Expected results:

The `document.open()` call is respected, and the page is cleared for a new document. This is the behavior in Chrome and Safari.

When `window.open()` is called in the inline script, there should be an active parser associated with the document.

window.stop() [1] is then defined to call the "abort a document" algorithm [2], which in its step 3 aborts [3] the active parser. At this point, the parser becomes no longer active, which is defined to be a parser "that has not yet been stopped or aborted" [4].

The crux of this issue lies in step 5 of "document open steps" in the HTML Standard [5], called by document.open().

> 5. If document has an active parser whose script nesting level is greater than 0, then return document.

When document.open() is called the parser is no longer active, so the condition should not be true, and the rest of the document open steps should be executed.

Yet, in Firefox's implementation, the aborted parser is still considered to be active, which is an incorrect interpretation of the spec. See [6]:

> // Note that aborting a parser leaves the parser "active" with its
> // insertion point "not undefined".

(Note, the comments reference an older version of the HTML Standard [7] which uses "insertion point is not undefined" instead of the current "script nesting level is greater than 0" condition (this was changed in [8]/[9]). The "active parser" part remains the same from the old spec to the current one though.)

Proposed WPT available at https://github.com/web-platform-tests/wpt/pull/11929.

[1]: https://html.spec.whatwg.org/multipage/window-object.html#dom-window-stop
[2]: https://html.spec.whatwg.org/multipage/browsing-the-web.html#abort-a-document
[3]: https://html.spec.whatwg.org/multipage/parsing.html#abort-a-parser
[4]: https://html.spec.whatwg.org/multipage/dom.html#active-parser
[5]: https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#document-open-steps
[6]: https://searchfox.org/mozilla-central/rev/c579ce13ca7864c5df9711eda730ceb00501aed3/dom/html/nsHTMLDocument.cpp#1249-1257
[7]: https://cdn.rawgit.com/whatwg/html/7b0112bbfb4923e8f64b0fd1c66d14fbd526fc33%5E/complete.html#dom-document-open
[8]: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17869
[9]: https://github.com/whatwg/html/commit/7b0112bbfb4923e8f64b0fd1c66d14fbd526fc33

Comment 1

[Boris Zbarsky [:bzbarsky]]

Sounds like the spec changed after our implementation?  Henri, can you take a look?

Comment 2

[Boris Zbarsky [:bzbarsky]]

How do various browsers behave after the user hits the "stop" button in the UI?

Comment 3

[Timothy Gu [:TimothyGu]]

> Sounds like the spec changed after our implementation?

Yes, but the "active parser" part never got changed (nor the definition of an active parser), which means that this has always been a valid bug.

Comment 4

[Timothy Gu [:TimothyGu]]

> How do various browsers behave after the user hits the "stop" button in the UI?

Given this HTML file:

------8<------
<p>This should get erased.</p>
<button onclick="document.open();">document.open()</button>
<script src="http://localhost:8888/slow.js"></script>
<script>
  alert("You were too slow! Hit stop before this message.");
</script>
------>8------

and this Node.js server:

------8<------
"use strict";

const http = require("http");

const server = http.createServer((req, res) => {
  if (req.url.includes("slow.js")) {
    setTimeout(() => {
      res.setHeader("Content-Type", "text/javascript");
        res.end("void 0;");
    }, 5000);
  } else {
    res.statusCode = 404;
    res.setHeader("Content-Type", "text/plain");
    res.end("Not found");
  }
});

server.listen(8888);
------>8------

Start up the Node.js server, load test.html from anywhere, hit stop before the page finishes loading, and then hit the "document.open()" button.

In Chrome and Safari the page gets cleared. In Edge, the page becomes unresponsive after the "document.open()" button is hit.

Comment 5

[Timothy Gu [:TimothyGu]]

(Note: in Edge the page becomes unresponsive after the "document.open()" button is hit, regardless of if the stop button was hit in time or not.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

This is still an issue even after bug 1489308 is fixed.

Comment 7

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

Attachment: Test case from comment 0

Comment 8

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

The mParserAborted check in document.open() was added to make https://mylith.com/mozilla/753278/ work. Simply removing it regresses that test case, which does show "Hello" in Chrome.

Comment 9

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

Maybe we should make nsDocShell::InternalLoad calling nsDocShell::Stop a more forceful stop than another call to nsDocShell::Stop. We could add STOP_EVER_MORE here: https://searchfox.org/mozilla-central/source/__GENERATED__/dist/include/nsIWebNavigation.h#108

Is there a better explanation for Chrome's behavior than navigation away via location stopping the document so forcefully as to prevent document.open but stop() stopping it less forcefully and allowing subsequent document.open?

Comment 10

[Anne (:annevk)]

Per the HTML standard, navigation effectively terminates the event loop (tasks belong to a no longer active document). window.stop() terminates fetching, but the event loop will still spin and activity can happen afterwards in response to user interaction for instance.

Hope that helps.

Comment 11

[Henri Sivonen (:hsivonen) (temporarily away from Bugzilla)]

(In reply to Anne (:annevk) from comment #10)

Per the HTML standard, navigation effectively terminates the event loop (tasks belong to a no longer active document). window.stop() terminates fetching, but the event loop will still spin and activity can happen afterwards in response to user interaction for instance.

Thanks. It seems we should make nsDocShell::InternalLoad mark the current doc as "navigation away started" and disable document.open if that flag has been set and not disable document.open if the "parser aborted" flag has been set. (Unclear if we even need the "parser aborted" flag in that case anymore.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

InternalLoad is the start of a navigation. It does not terminate the event loop per spec; it just does https://html.spec.whatwg.org/multipage/browsing-the-web.html#abort-a-document which includes:

If document has an active parser, then abort that parser and set document's salvageable state to false.

which calls into https://html.spec.whatwg.org/multipage/parsing.html#abort-a-parser

Also per spec, https://html.spec.whatwg.org/multipage/window-object.html#dom-window-stop calls https://html.spec.whatwg.org/multipage/browsing-the-web.html#stop-document-loading which just calls https://html.spec.whatwg.org/multipage/browsing-the-web.html#abort-a-document

So per spec, these should have the same exact effect on the parser, as far as I can tell. And that effect, again as far as I can tell, should be that the parser should be considered done and open() and write() should work as if after DOMContentLoaded.

So in the testcase at https://mylith.com/mozilla/753278/ I would expect the sequence of events to be as follows per spec:

1. The location set terminates the parser.
2. The open() call cancels the location-set-started load, opens the doc.
3. The write() call writes stuff.

So "Hello" should NOT appear per spec as currently written. Timothy, do you know what is Chrome actually doing here? Because it doesn't seem to match the spec afaict. The spec does talk about "active parser whose script nesting level is greater than 0" but https://html.spec.whatwg.org/multipage/parsing.html#abort-a-parser will make the parser not active.

Is this some situation where Chrome actually processes .location sets async instead of doing what the spec says, maybe? If the location set is doing all its work after the open() call, that would explain the Chrome behavior in this testcase... :(

Comment 13

[Timothy Gu [:TimothyGu]]

Is this some situation where Chrome actually processes .location sets async instead of doing what the spec says, maybe?

That’s correct. Blink’s Location.href setter calls into Frame::ScheduleNavigation, which queues a task to navigate rather than navigating directly. More in general, I want to say that all “normal” navigations in Chrome are asynchronous.

Comment 14

[Boris Zbarsky [:bzbarsky]]

OK. That's definitely not what the spec says. Is there a Chrome bug tracking this, do you know?

Comment 15

[Timothy Gu [:TimothyGu]]

I asked Nate Chapin and he mentioned
https://bugs.chromium.org/p/chromium/issues/detail?id=914587 as the bug for that issue. Additionally he’s actively working on making Location-object navigation (and initial iframe load) synchronous as per spec.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Awesome, thank you!

Comment 17

[Firefox Bug Husbandry Bot]

See bug 1547409. Moving webcompat whiteboard tags to project flags.

Comment 18

[Boris Zbarsky [:bzbarsky]]

OK, it looks like Chrome has changed its behavior to sync-navigate from location.href sets, so the testcase in comment 8, which has this basic form:

   <script>
     location.href = "data:text/html,<h1>Hello</h1>";
     document.open();
   </script>

Now shows a blank page in Chrome dev builds (when run inside a subframe, so the navigation to data: actually works in general). That's what the spec says should happen, and I think we should align with it.

Comment 19

[Boris Zbarsky [:bzbarsky]]

That said, doing that without adding a "planned navigation" async thing for form submission would make us have the web compat problem described at https://bugs.chromium.org/p/chromium/issues/detail?id=955556.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Per discussion in https://github.com/whatwg/html/issues/4723 it looks like we're going to align on our current behavior.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I filed bug 1580381 to implement the test changes to account for that likely spec change.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Karl, we may need to get the webcompat issue fixed some other way, but Chrome aligning with us here should help, hopefully...

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch I was considering when I was thinking of changing the behavior

Comment 24

[Karl Dubost💡 :karlcow]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #22)

Karl, we may need to get the webcompat issue fixed some other way, but Chrome aligning with us here should help, hopefully...

Understood. Thanks for the heads up.
Reopened https://webcompat.com/issues/28103#issuecomment-530205507 and switched to needscontact