Last Comment Bug 332848 - document.write sorta but not really supported in XHTML
: document.write sorta but not really supported in XHTML
Status: RESOLVED FIXED
[sg:want]
: fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Linux
-- normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-05 10:00 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2008-11-01 02:37 PDT (History)
12 users (show)
timr: blocking1.8.0.4+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
I say we do this (2.98 KB, patch)
2006-04-05 15:44 PDT, Boris Zbarsky [:bz] (still a bit busy)
mrbkap: review+
jst: review+
dveditz: superreview+
dveditz: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-05 10:00:07 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-05 10:03:19 PDT
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 User image Tim Riley [:timr] 2006-04-05 11:48:28 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-05 15:44:42 PDT
Created attachment 217346 [details] [diff] [review]
I say we do this
Comment 4 User image Blake Kaplan (:mrbkap) 2006-04-05 15:50:09 PDT
Comment on attachment 217346 [details] [diff] [review]
I say we do this

r=mrbkap
Comment 5 User image Daniel Veditz [:dveditz] 2006-04-05 17:35:52 PDT
Comment on attachment 217346 [details] [diff] [review]
I say we do this

sr=dveditz
Comment 6 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-05 17:38:15 PDT
Comment on attachment 217346 [details] [diff] [review]
I say we do this

This really needs ok from jst...
Comment 7 User image Jesse Ruderman 2006-04-05 22:18:12 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-05 22:28:23 PDT
> 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 User image georgi - hopefully not receiving bugspam 2006-04-05 23:19:40 PDT
(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 User image Jesse Ruderman 2006-04-05 23:34:14 PDT
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 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-05 23:36:52 PDT
No, document.open() does not create a new document, it starts modification of the existing one.
Comment 12 User image georgi - hopefully not receiving bugspam 2006-04-05 23:58:48 PDT
(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 User image Jesse Ruderman 2006-04-06 00:05:13 PDT
> 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 User image Boris Zbarsky [:bz] (still a bit busy) 2006-04-06 08:50:17 PDT
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 User image georgi - hopefully not receiving bugspam 2006-04-06 09:18:16 PDT
sorry

as usual i have misunderstood :)
Comment 16 User image Daniel Veditz [:dveditz] 2006-04-28 12:34:30 PDT
jst: we need your review so we can land this.
Comment 17 User image Johnny Stenback (:jst, jst@mozilla.com) 2006-05-02 19:25:42 PDT
Comment on attachment 217346 [details] [diff] [review]
I say we do this

r=jst
Comment 18 User image Daniel Veditz [:dveditz] 2006-05-03 10:47:04 PDT
Comment on attachment 217346 [details] [diff] [review]
I say we do this

approved for 1.8.0 branch, a=dveditz
Comment 19 User image Boris Zbarsky [:bz] (still a bit busy) 2006-05-03 14:26:22 PDT
Fixed, trunk and both branches.
Comment 20 User image Boris Zbarsky [:bz] (still a bit busy) 2007-01-17 16:26:58 PST
Added mochitest.
Comment 21 User image georgi - hopefully not receiving bugspam 2008-11-01 02:37:41 PDT
bindings allow to embed XUL in html (quirks mode).

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

Note You need to log in before you can comment on or make changes to this bug.