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] (Out June 25-July 6)
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-05 10:00 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
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] (Out June 25-July 6)
mrbkap: review+
jst: review+
dveditz: superreview+
dveditz: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 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 Boris Zbarsky [:bz] (Out June 25-July 6) 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 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 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-04-05 15:44:42 PDT
Created attachment 217346 [details] [diff] [review]
I say we do this
Comment 4 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-04-05 15:50:09 PDT
Comment on attachment 217346 [details] [diff] [review]
I say we do this

r=mrbkap
Comment 5 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 Boris Zbarsky [:bz] (Out June 25-July 6) 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 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 Boris Zbarsky [:bz] (Out June 25-July 6) 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 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 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 Jonas Sicking (:sicking) PTO Until July 5th 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 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 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 Boris Zbarsky [:bz] (Out June 25-July 6) 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 georgi - hopefully not receiving bugspam 2006-04-06 09:18:16 PDT
sorry

as usual i have misunderstood :)
Comment 16 Daniel Veditz [:dveditz] 2006-04-28 12:34:30 PDT
jst: we need your review so we can land this.
Comment 17 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 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 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-05-03 14:26:22 PDT
Fixed, trunk and both branches.
Comment 20 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-01-17 16:26:58 PST
Added mochitest.
Comment 21 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.