Closed Bug 1489308 Opened Last year Closed 9 months ago

Align document.open() with overhauled spec

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: timothygu99, Assigned: bzbarsky)

References

(Blocks 8 open bugs, Regressed 4 open bugs, )

Details

(4 keywords, Whiteboard: [webcompat:p1][wptsync upstream])

Attachments

(10 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Blocks: 1449992
Blocks: 1456313
Flags: needinfo?(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.

Flags: needinfo?(bobbyholley)
Flags: needinfo?(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.)

Flags: needinfo?(annevk)

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

Flags: needinfo?(bobbyholley)

So I would expect them to always match.

Good catch. OK, that makes sense.

Blocks: 1519333
Depends on: 680086
Blocks: 1334657
Blocks: 1495175
Blocks: 1520333
Flags: webcompat? → webcompat+
Whiteboard: [webcompat:p1]
Blocks: 1271025
Blocks: 1475000
Blocks: 1520804
Blocks: 1520831

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

Assignee: nobody → mcaceres

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...

Assignee: mcaceres → bzbarsky
Flags: needinfo?(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.

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.

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

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.

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.

Duplicate of this bug: 1456313
Blocks: 1470017
Blocks: 680086
No longer depends on: 680086
Depends on: 1528146
Blocks: 1520075

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?

Flags: needinfo?(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.

Flags: needinfo?(bzbarsky)
Blocks: 1528448
Blocks: 1528449

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.

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

Blocks: 1048349

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

Blocks: 223147
Blocks: 465961
Blocks: 472895
Blocks: 497644
Blocks: 521918
Blocks: 528925
Blocks: 613578
Blocks: 627001
Blocks: 1490129
Blocks: 835613

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

Keywords: dev-doc-needed
Blocks: 1515769
Blocks: 716012
Blocks: 1357369
Blocks: 1460939
Blocks: 1347608
Blocks: 1417566
Blocks: 1107113
Blocks: 1371276
Blocks: 725424
Blocks: 1346600
Blocks: 788547
Depends on: 1531128
Pushed by bzbarsky@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15641 for changes under testing/web-platform/tests
Whiteboard: [webcompat:p1] → [webcompat:p1][wptsync upstream]
Depends on: 1531860
Regressions: 1544670
No longer regressions: 1544670

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!

Flags: needinfo?(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.

Flags: needinfo?(bzbarsky)
Blocks: 81980
Blocks: 1311694
Blocks: 325352
Blocks: 717339
Blocks: 823196
Blocks: 1332241
Blocks: 1368005
Blocks: 1453721
Blocks: 1463757

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.

Duplicate of this bug: 1548957

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

Regressions: 1554014
Regressions: 1572798
Regressions: 1575799
Regressions: 1592238
You need to log in before you can comment on or make changes to this bug.