332848 - document.write sorta but not really supported in XHTML

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

In XHTML right now, if you do a document.write on a document that's done parsing, it will work (and parse with the tagsoup parser).  If you then call document.write again (on the still-open document) it will throw.

Oh, and calling Write() will try to terminate mParser in some cases before ever checking for XHTML.

And Close() will kill the parser in XHTML.

Why are we playing the games with checking for mParser before throwing and so forth?  Imo, the first thing in Close/OpenCommon/WriteCommon should be a check for XHTML and a throw if that tests true...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that I ran across this in bug 332807.  In general, allowing our parser to be messed with like this in XML is scary to me -- it's not well-tested at all.

Comment 2

[Tim Riley [:timr]]

We need an owner to investigate this.  We are plus'ing it for now but might reconsider if we can't get action on it.  a=timr for drivers.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: I say we do this

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 217346 [details] [diff] [review]
I say we do this

r=mrbkap

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 217346 [details] [diff] [review]
I say we do this

sr=dveditz

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 217346 [details] [diff] [review]
I say we do this

This really needs ok from jst...

Comment 7

[Jesse Ruderman]

I'm worried that this change will break some bookmarklets and web sites.  Why not fix document.open and document.write to work correctly instead?  Is the problem that the XML parser doesn't like to be terminated early?

(I think "work correctly" means: document.write would throw if used during XHTML page load, document.open would create a new text/html or text/plain document as requested, and document.write by itself would imply document.open('text/html').)  

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Is the problem that the XML parser doesn't like to be terminated early?

Yes.  Or rather, that termination via close() or write() does weird stuff to not only the parser but also the DOM and makes assumptions about the DOM that are false for XML.  I'm not so interested in mutating the type of our document from XHTML to HTML...  If someone wants to do that, sure, but I really don't have the time to write something like that and test it properly, including dealing with the above assumptions.

Note that the only way things can work now is if you wait till the document is loaded and then issue one single write() call and nothing else.  Anything else will throw.  At least in terms of adding data to the document...

Comment 9

[georgi - hopefully not receiving bugspam]

(In reply to comment #0)
> In XHTML right now, if you do a document.write on a document that's done
> parsing, it will work (and parse with the tagsoup parser).  If you then call
> document.write again (on the still-open document) it will throw.
> 

i may be missing something, but in bug 332807 there is no need for document.write to achieve a crash - check 
https://bugzilla.mozilla.org/attachment.cgi?id=217383
and bug 332807 works for text/html.

Comment 10

[Jesse Ruderman]

I'm confused.  Doesn't document.open() create a new document?  So why does it break when the previous document was XHTML, and you call document.write twice?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, document.open() does not create a new document, it starts modification of the existing one.

Comment 12

[georgi - hopefully not receiving bugspam]

(In reply to comment #10)
> I'm confused.  Doesn't document.open() create a new document?  So why does it

i am not competent enough to comment, but it seems irrelevant what document.open() creates. the interesting part is what it *destroys* afaict.

> break when the previous document was XHTML, and you call document.write twice?
> 

the two bugs got mixed. in
https://bugzilla.mozilla.org/attachment.cgi?id=217382
working from *html* and xhtml there is no document.write()

Comment 13

[Jesse Ruderman]

> No, document.open() does not create a new document, it starts modification of
> the existing one.

I guess I was confused by the fact that it does create a new session history entry.  The previous document not bfcached unless you go back and forth a few times.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Georgi, this bug is not the same as bug 332807.  This bug is its own set of problems, completely unrelated to the XBL mess that is bug 332807.

And in fact, what matters in this bug is not what document.open() destroys, imo, but _how_ it destroys it.  And whether other code expects that to happen.

Comment 15

[georgi - hopefully not receiving bugspam]

sorry

as usual i have misunderstood :)

Comment 16

[Daniel Veditz [:dveditz]]

jst: we need your review so we can land this.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 217346 [details] [diff] [review]
I say we do this

r=jst

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 217346 [details] [diff] [review]
I say we do this

approved for 1.8.0 branch, a=dveditz

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed, trunk and both branches.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Added mochitest.

Comment 21

[georgi - hopefully not receiving bugspam]

bindings allow to embed XUL in html (quirks mode).

is xul:iframe.parent == HTMLDocument considered safe ?