1489308 - Align document.open() with overhauled spec

Status: RESOLVED FIXED

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

Description

[Timothy Gu [:TimothyGu]]

The model for document.open() has recently been drastically simplified in the WHATWG HTML Standard. See https://github.com/whatwg/html/issues/3818. The linked issue lists a series of Web Platform Tests pull requests, all of which contribute to the now up-to-date test suite in https://github.com/web-platform-tests/wpt/tree/master/html/webappapis/dynamic-markup-insertion/opening-the-input-stream.

The primary changes applicable to Gecko are:

- A new JavaScript realm is no longer created (https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/no-new-global.window.js)
- Tasks are no longer canceled (https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/tasks.window.js)
- Document is set to no-quirks mode (https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/quirks.window.js)
- Event listeners are removed from every node in the DOM tree AND the Window object (https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/event-listeners.window.js)
- Non-active documents are now permitted to call document.open() (https://github.com/web-platform-tests/wpt/blob/master/html/webappapis/dynamic-markup-insertion/opening-the-input-stream/active.window.js)
- The document is no longer aborted, UNLESS there is an active or queued navigation (https://github.com/web-platform-tests/wpt/pull/10789)

There are some other minor changes that I didn't list, all of which are tested by WPTs.

See also the independently tracked bug 1475000.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So do I understand correctly that the new setup changes the document URL but does not change the document's origin, so the two might no longer match in the common case of both being http URLs, if document.domain is involved?

Does anyone actually implement this so far? It seems pretty questionable to me....

I guess mutating the origin is a bit questionable too.

Comment 2

[Anne (:annevk)]

Note that there's also:

If document's origin is not same origin to entryDocument's origin, then throw a "SecurityError" DOMException.

So I would expect them to always match. (That step was added by jeisinger from Google at some point, preceding this larger rewrite. And I'm pretty sure they implement this same origin check in some fashion.)

Comment 3

[Bobby Holley (:bholley)]

bz and I just chatted live, and he's good to go here.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So I would expect them to always match.

Good catch. OK, that makes sense.

Comment 5

[Marcos Caceres [:marcosc]]

I'mma take this, but can't start for a week or so.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sorry, I should have commented in here and just taken this bug... I have patches for this. They are green on try except for one issue involving webextension content scripts that I'm still thinking about how to address (and sort of waiting on Kris for tests).

As it turned out, this was probably not a great but for someone not already very familiar with how our load-event-firing code worked, because getting that part right for the new document.open setup was by far the most complicated bit...

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm going to post the patches so they're available to look at, but not ask for review for now until the webextension issue is sorted out, I guess.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1489308 part 1. Factor out part of nsDocShell::AddState into a separate method. r=qdot

This implements the "URL and history update steps" from the HTML spec. See
https://html.spec.whatwg.org/multipage/history.html#url-and-history-update-steps.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1489308 part 2. Allow UpdateURLAndHistory to work even if mOSHE is null, if we're doing a replace. r=smaug

We're going to end up hitting this if someone does a document.open() before mOSHE
has been set. We shouldn't need to worry about mLSHE, because the document.open()
will cancel the corresponding load.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1489308 part 3. Add a public RemoveAllListeners method on EventListenerManager. r=smaug

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1489308 part 4. Allow UpdateURLAndHistory to work even if there's no root session history. r=smaug

This can happen when someone does a document.open() on a document that has no session history.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1489308 part 5. Align the work we do on document.open with the spec. r=mccr8,smaug

The change to browser_modifiedclick_inherit_principal.js is because we no
longer set the docshell to a wyciwyg URL when document.open() happens and the
test was depending on that to terminate.

browser_wyciwyg_urlbarCopying.js is being removed because it's trying to test
wyciwyg URIs, which no longer exist.

The changes in docshell/test/navigation are because document.open() no longer
affects session history. One of the tests was testing the interactions there
and is being removed; another is being repurposed to just test that
document.open() does not affect history.length.

The change to test_x-frame-options.html is because document.open() now removes
even listeners on the window, which it didn't use to do (and in the specific
case in this test reused the existing inner too, so the listener was still
around in practice). The new behavior matches other browsers.

The removal of test_bug172261.html is because document.open() no longer affects
session history, so you can't go back across it or forward to the "opened"
state, so the situation that test is trying to test no longer exists.

The changes to test_bug255820.html are because reloading a document after
document.open() will now just load the URL of the document that was the entry
document for the open() call, not reload the written content. So there's not
much point testing reload behavior, and in this test it was just reloading the
toplevel test file inside the frames.

The change to test_bug346659.html is because now we no longer create a new
Window on document.open().

The change to test_bug1232829.html is because document.open() (implicit in this
test) no longer adds history entries, so the back() was just leaving the test
page instead of going back across the document.open(). The test is a
crashtest in practice, so might still be testing something useful about how
document.open() interacts with animations.

The change to test_bug715739.html is because the URL of the document after
document.open() is now the URL of the entry document, not a wyciwyg URL, so
reload() has different behavior than it used to.

The change to test_bug329869.html is because now when we go back we're
reloading the original document we had, not doing a wyciwyg load, and the
security info now doesn't include the untrusted script.

The changes to the wpt expectations are removing a bunch of expected failures
now that we pass those tests and disabling some tests that are fundamentally
racy and hence fail randomly. The latter all have github issues filed for the
test problem.

The change to testing/web-platform/tests/common/object-association.js is fixing
tests that were not matching the spec (and were failing in other browsers).

The change to parser-uses-registry-of-owner-document.html is fixing tests that
were not matching the spec (and were failing in other browsers).

The change to document-write.tentative.html is because the test was buggy: it
was using the same iframe element for all its tests and racing loads from some
tests against API calls from other tests, etc. It's a wonder it ever managed
to pass, independent of these patches (and in fact it doesn't pass according to
wpt.fyi data, even in Firefox).

The changes in html/browsers/history/the-history-interface are because
document.open() no longer adds history entries. The test was failing in all
other browsers for the same reason.

The changes in html/browsers/history/the-location-interface are because
reloading a document.open()-created thing now loads the URL of the page that
was the entry document for the open() call. The test was failing in all other
browsers.

The change to reload_document_open_write.html is beause we now reload the url
of the document that entered the script that called open() when we reload, not
the written content. Other browsers were failing this test too; Gecko with
the old document.open implementation was the only one that passed.

The change to http-refresh.py is to fix a test bug: it was not returning a
Content-Type header, so we were putting up helper app dialogs, etc.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1489308 part 6. Remove now-unused mWillReparent member. r=mccr8

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1489308 part 7. Remove now-unused mDidDocumentOpen member. r=mccr8

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1489308 part 8. Remove unneeded JSContext args on open/write/writeln. r=mccr8

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1489308 part 9. Remove now-unused wyciwyg bits. r=mccr8

Comment 17

[Marcos Caceres [:marcosc]]

Sorry, I should have commented in here and just taken this bug... I have patches for this.

No problem - and wowser!... yeah, looking at the patches, I would have been out of my depth for this one. I've got some other bugs to work on, so all good.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Bug 1489308 part 10. Remove some document.open handling in outer window that's no longer needed. r=mccr8

Comment 20

[Andrew McCreight [:mccr8]]

I haven't read through all of these patches yet, but why are you removing wyciwyg in these patches? I didn't see any justification for that in the commit messages for part 5 and part 9. Is this just because it is some weird Gecko-ism where the implementation is mostly in document.open anyways, so you might as well remove it at the same time (or even removing it simplifies the other work)? These patches are pretty big already so it feels like there needs to be some justification for doing that at the same time. Is removing wyciwyg going to increase the web compat risk of these patches? Or are those URLs already basically banned from web content already?

Comment 21

[Boris Zbarsky [:bzbarsky]]

but why are you removing wyciwyg in these patches?

The only thing wyciwyg is used for is to allow session history navigation (and reload) across document.open/write/close.

That is, in Gecko right now when you do document.open() we do the following things:

1. Create a new Window.
2. Create a new session history entry.
3. Create a wyciwyg channel. When document.write() happens, we both parse that data
   and save it inside the channel's cache entry. When document.close() happens
   we close the cache entry.
4. If you now navigate to a session history entry that has a wyciwyg URI, we use that
   to look up the cache entry and read the data from there. So the session history behavior
   is sort of as if you had reexecuted the open(), write(), close() bits from before. Or at
   least you get the same HTML as was written before.

In the new document.open() model, the one in the spec, there is no new Window, no new session history entry, no magic around session history. You just clear the document, change its URL, and munge its DOM. If you reload, or otherwise navigate through history, the navigation is done based on the (changed) URL. It won't reload the data that was written, but whatever the URL points at. So in the new document.open() model there is no need at all for wyciwyg. After part 5 it's all dead code.

Or are those URLs already basically banned from web content already?

Correct. That's what https://searchfox.org/mozilla-central/rev/8e8ccec700159dc4efe061cfec2af10b21a8e62a/docshell/base/nsDocShell.cpp#9146-9151 is enforcing.

Comment 22

[Andrew McCreight [:mccr8]]

For part 7, please add a brief explanation as to why it is okay to remove the code. The other cleanup patches I looked at were clearly just removing completely dead code, but that one seems to require some broader context about how document open will never change the parent now. The over all bug summary "Align document.open() with overhauled spec" does not make that obvious, and you can sort of figure it out by digging through the giant laundry list in comment 0, but it would be nice to have it explicitly spelled out for that patch. As a non-expert on this code, it was not obvious at a glance until I looked over both comment 0 and the patch a few times.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Good catch, and sorry for not making that clear. I'll give part 7 a better commit message.

Comment 24

[Andrew McCreight [:mccr8]]

There are 29 open bugs with Wyciwyg in the summary. It might be worth going through those once this has landed.

Comment 25

[Chris Mills [:cmills]]

Check to see if any docs updates are needed to make sure the new behaviour is accurately described.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f00a2d89b5f
part 1.  Factor out part of nsDocShell::AddState into a separate method.  r=qdot
https://hg.mozilla.org/integration/autoland/rev/f2b0ceb55ced
part 2. Allow UpdateURLAndHistory to work even if mOSHE is null, if we're doing a replace.  r=smaug
https://hg.mozilla.org/integration/autoland/rev/b393e04da7e9
part 3.  Add a public RemoveAllListeners method on EventListenerManager.  r=smaug
https://hg.mozilla.org/integration/autoland/rev/015db4a424d9
part 4.  Allow UpdateURLAndHistory to work even if there's no root session history.  r=smaug
https://hg.mozilla.org/integration/autoland/rev/6504b5468b32
part 5.  Align the work we do on document.open with the spec.  r=mccr8,smaug
https://hg.mozilla.org/integration/autoland/rev/c1113b00d864
part 6.  Remove now-unused mWillReparent member.  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/2166dac4d26c
part 7.  Remove now-unused mDidDocumentOpen member.  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/8e1196296ad4
part 8.  Remove unneeded JSContext args on open/write/writeln.  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/5fbeaa1c3997
part 9.  Remove now-unused wyciwyg bits.  r=mccr8
https://hg.mozilla.org/integration/autoland/rev/a01586b62cf5
part 10.  Remove some document.open handling in outer window that's no longer needed.  r=mccr8

Comment 27

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6f00a2d89b5f
https://hg.mozilla.org/mozilla-central/rev/f2b0ceb55ced
https://hg.mozilla.org/mozilla-central/rev/b393e04da7e9
https://hg.mozilla.org/mozilla-central/rev/015db4a424d9
https://hg.mozilla.org/mozilla-central/rev/6504b5468b32
https://hg.mozilla.org/mozilla-central/rev/c1113b00d864
https://hg.mozilla.org/mozilla-central/rev/2166dac4d26c
https://hg.mozilla.org/mozilla-central/rev/8e1196296ad4
https://hg.mozilla.org/mozilla-central/rev/5fbeaa1c3997
https://hg.mozilla.org/mozilla-central/rev/a01586b62cf5

Comment 28

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

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

Comment 29

[Chris Mills [:cmills]]

I looked at this bug, and wasn't sure if any specific new web developer documentation was needed in light of it. It mostly sounds like internals stuff. Let me know if I'm wrong.

I did update the document.open() page because it was out of date and was missig Examples and several other sections:
https://developer.mozilla.org/en-US/docs/Web/API/Document/open

Ca you have a look and let me know if this is looking OK?

Thanks!

Comment 30

[Boris Zbarsky [:bzbarsky]]

Chris, thank you for looking at the docs end!

This change pretty radically modified the behavior of document.open (how it interacts with session history, which arguments it actually pays attention too, what side-effects it has), but it might not matter too much if the old thing wasn't really documented, and it looks like it wasn't.

As far as the docs at https://developer.mozilla.org/en-US/docs/Web/API/Document/open go, I have a few comments:

1. The two forms of document.open() that the page discusses do totally different things: the three-arg form is just an alias for window.open() while the no-arg form is the one this bug was about. I'm not sure how best to explain that in the MDN page. The three-arg form is pretty much never what people want; I would suggest its documentation should be a footnote or aside, not the main focus of the page as now. Just documenting that it does the same thing as window.open() and linking to those docs is probably best. Then the rest of the article can focus on the no-arg version.

2. The return value of the two forms is different: the three-arg form returns a WindowProxy, not a Document. Again, this is probably best handled by changing how we document the three-arg form.

3. The "normal" (not 3-arg) form doesn't ever throw InvalidAccessError or SyntaxError that I can see. It can throw InvalidStateError and SecurityError, though, in various cases.

4. "simply opens the document" has some side-effects like removing all event listeners currently registered on the document, the nodes in it, and the window, and removing all existing nodes from the document.

5. The fact that document.write "after he page has loaded" calls open() is in fact clearly defined in the spec, while the "Notes" section claims otherwise. See https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#document-write-steps step 3.2.

6. As far as the compat table goes, Gecko only supports this on HTMLDocument, not Document. I just filed bug 1549560 on this, though in practice open() throws for non-HTML documents (https://html.spec.whatwg.org/multipage/dynamic-markup-insertion.html#document-open-steps step 1), so it's not that big a deal.

Comment 31

[Timothy Gu [:TimothyGu]]

Chris, thank you too for updating MDN! I think it might help for users also to document the historical signature of document.open(type, replace), which is now forwarded to just document.open(). Additionally, for years Firefox (and Internet Explorer) had a different behavior where it would erase all JavaScript variables, etc., in addition to just erasing page content. This all seems like useful information.

Comment 33

[Chris Mills [:cmills]]

OK, thanks to Boris and Timothy for thhe comments. I have responded to your feedback by rewriting and adding in various bits of content. I hope this now reads better:

https://developer.mozilla.org/en-US/docs/Web/API/Document/open