Remove processing for document.open()'s type parameter

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: annevk, Assigned: bzbarsky)

Tracking

({site-compat})

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

It's not implemented in Chrome, Edge, or Safari.

HTML Standard: https://github.com/whatwg/html/pull/3559.
bz, this is a simplification we could make of document.open() that doesn't touch the bits we're less clear on. (I do still plan on researching those more.)
Priority: -- → P3
Hmm.  At one point this was definitely used.  Worse yet, things that do use it can end up with security bugs due to a change from things being treated as text to things being treated as HTML...

Are we _very_ very sure no one else supports this?  IE used to, but they removed it in edge?
Flags: needinfo?(annevk)
OK, just tested, and https://jsfiddle.net/s7Lhdkxh/ shows that Edge and IE both don't support it.  OK, let's nix it.
Assignee: nobody → bzbarsky
Flags: needinfo?(annevk)
So this introduces a new parsing mode that I think didn't use to exist in the spec: parsing an entire input document as text/html while a document is text/plain.

Testcase: https://jsfiddle.net/s7Lhdkxh/3/ or http://web.mit.edu/bzbarsky/www/testcases/document/document-open-2.html

And there's no wpt test coverage around this, afaict.... nor around what happens if you call document.write() in a text/plain document that is still loading.
Flags: needinfo?(d)
Flags: needinfo?(annevk)
I am going to take a guess that document.write() should continue parsing as plaintext when done during the load of a plaintext document, and proceed on that basis.  Someone needs to write a test for that.
Comment on attachment 8962879 [details] [diff] [review]
Remove support for the 'type' parameter to document.open

That has test failures....
Attachment #8962879 - Flags: review?(kyle)
Comment on attachment 8962879 [details] [diff] [review]
Remove support for the 'type' parameter to document.open

Review of attachment 8962879 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +12266,5 @@
>    }
>  
>    mCachedEncoder = nullptr;
>    mContentType = aType;
> +  mContentTypeForWriteCalls = aType;

nit: Kinda confused by this addition. The only place mContentTypeForWriteCalls is used appears to be after the mContentTypeForWriteCalls.AssignLiteral in nsHTMLDocument::Open (which happens after the call to SetContentTypeInternal), so isn't this just going to get reset no matter what? Just trying to make sure I understand the logic flow here, don't think it affects anything though.
Attachment #8962879 - Flags: review+
> after the mContentTypeForWriteCalls.AssignLiteral in nsHTMLDocument::Open

It's used in Close() and Write().  You can do Write() without doing Open().  That's the whole point: I didn't want to hardcode Write() always parsing as HTML, in case someone does a Write() mid-load on a text document.
Ok, that's what I was missing, thanks!
I guess the test failures are all for the tests for bug 659763.
> I am going to take a guess that document.write() should continue parsing as plaintext when done during the load of a plaintext document, and proceed on that basis.

It's unclear to me how this can be tested. Won't the insertion point be undefined? I tried to trickle a text/plain load and then document.write() into it, but that'll always replace the contents of the document.

I'll add some tests about invoking document.write() without having invoked document.open() though. That should generally do the same as if you had invoked document.open() first, but it'd be good to test.

As for document's content type, should we consider updating it in the "document open steps" to "text/html"? All browsers agree on it staying "text/plain" today, but their behavior is not aligned with that.
I created https://github.com/w3c/web-platform-tests/pull/10209 to test document.write() on an image/png and text/plain document. For some reason it fails in Firefox (times out on loading some resources), but works fine in Chrome and Safari (who do fail rather vaguely on the image/png case but at least without a timeout). Hope this is what comment 5 needed.

(Note that for now I just tested that contentType remains untouched. We could change that if we wanted to, but since nothing really depends on what it says, I'm not sure it's worth it.)
Flags: needinfo?(d)
Flags: needinfo?(annevk)
Just like the previous version, exept for the test fixes
Attachment #8963137 - Flags: review?(kyle)
> I created https://github.com/w3c/web-platform-tests/pull/10209 

Thank you.  That tests the behavior when doing a write-after-load (with an implicit open).  It would be interesting, if possible, to test a write-during-load... that's the one I could not figure out how to test.

> For some reason it fails in Firefox (times out on loading some resources)

Because the test doesn't document.close() the documents it opens, so they're never done loading.

> but since nothing really depends on what it says

Last I checked, there were parts of the browser UI in various browsers that were enabled or not depending on the content type of the thing being viewed.  But I last checked a while ago.
Attachment #8963137 - Flags: review?(kyle) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72cdb625f0ce
Remove support for the 'type' parameter to document.open.  r=qdot
> It would be interesting, if possible, to test a write-during-load...

I don't know if you saw, but I speculated a bit about this on IRC and I don't think it's possible. The only way to get an insertion point is with a script end tag or document.open() call.

And if you move an element then I think it's still the original parser in control, though I could not really get Firefox to execute the second script here using Live DOM Viewer so that avenue of testing is also closed (probably worthy of further study though):

  <iframe src=image></iframe>
  <div>
   <script>frames[0].document.body.appendChild(document.currentScript.parentNode);</script>
   <script>document.write("test")</script>
  </div>

> Last I checked, there were parts of the browser UI in various browsers that were enabled or not depending on the content type of the thing being viewed.

Do you remember where? Right click does not really show anything for

  http://software.hixie.ch/utilities/js/live-dom-viewer/?%3Ciframe%20src%3Dimage%3E%3C%2Fiframe%3E

However, given that you can also manipulate the page through DOM manipulation I no longer see the point of resetting document's content type for this legacy API.
https://hg.mozilla.org/mozilla-central/rev/72cdb625f0ce
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
> Do you remember where? 

There's various bits in the context menu and find code in Firefox, but it mostly seems to be about text/* vs not, so treats HTML and plaintext identically.  See https://searchfox.org/mozilla-central/search?q=mimeTypeIsTextBased

In other browsers I recall issues around whether devtools were enabled and such, but I don't recall details and they might have changed behavior.

The main worry here, of course, is sites effectively disabling parts of the browser UI in user-hostile ways.  And yes, the fact that you can just inject stuff via direct DOM manipulation does make this less of a concern, I agree.
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10305 for changes under testing/web-platform/tests
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.