503473 - [HTML5] Prevent document.write() where prohibited by HTML5

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Henri Sivonen (:hsivonen)]

The HTML5 parser now inherits the old parser's document.write() allowance behavior. Need to make it comply to HTML5 (making it more IE-like).

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Attempt at implementing black-box equivalence with insertion point except where intentionally different

Intentional differences: SVG script is not special. Defer scripts get to document.write().

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: New iteration

I think I'm still failing to get the insertion point behavior right in the case where there are document.written scripts and a script-created external script and a parser-created external script are competing for their run slot.

This turned out to be harder than I thought.

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Hopefully worthy of check-in

Need to test this a bit before asking for r.

Comment 4

[Henri Sivonen (:hsivonen)]

The patch is probably wrong for the case when a script-inserted external script document.writes when the parser is script-created. I don't know yet, how to fix that. The script-inserted external script would need to get the root context key as its parser key, basically.

Comment 5

[Henri Sivonen (:hsivonen)]

sicking, I think the patch is now check-in-worthy even though it doesn't seem to address the harder corner cases properly. (I think that I need some Hixie-endorsed expected results for http://hixie.ch/tests/adhoc/dom/level0/write/005.html before I tweak this further.)

The general thinking is this:

 * If the tree builder is flushing, we can't be at a point where the conceptual
parser from the spec is looking at a </script> end tag, because at that point
the flush is already over in this implementation. Maybe this should be an
assert than an opt-build-level check, but I'm not confident yet that a flush
can't cause event handlers of any kind to run.

 * When the parser is script-created, there's always a defined insertion point.
However, with this patch, document.write() is likely to insert into the wrong
insertion point when document.write() is called from something that isn't a
parser-inserted script and isn't the script that initially called
document.open(). (Basically, such calls should somehow be made to inherit the
root context key.)

 * When a parser-inserted script is run, the insertion point is defined.

 * The script elements hold a weak reference to the creator parser, because it
would be wrong to establish an insertion point in the parser of another
document if the script's ancestor has gotten moved into another document that
also happens to have an active parser.

 * defer scripts intentionally count towards establishing an insertion point
due to
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2009-July/020843.html

Potential problems:
 * Is it possible that:
   - a non-external parser-inserted script has incremented the counter for
parser-inserted scripts being executed
   - the script document.writes an external script
   - the event loop spins
   - something other than the external script gets run from the event loop and
the other thing gets to perform a document.write as if it were the pending
external script
 ?

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Try to be more correct with non-parser-inserted scripts when there's a script-created parser

Trying to address the case where a script that is neither inserted by the active parser nor the script that called document.open() does a document.write() on a previously document.open()ed doc.

Comment 7

[Henri Sivonen (:hsivonen)]

If this gets r&sr, feel free to treat this as checkin-needed.

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 388899 [details] [diff] [review]
Try to be more correct with non-parser-inserted scripts when there's a script-created parser

Bitrotted. :-(

Comment 9

[Henri Sivonen (:hsivonen)]

Note to self: Remember to check if bug 516584 is fixed by the fix for this bug.

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: Unrot

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Refresh patch

Comment 12

[Henri Sivonen (:hsivonen)]

Attachment: Forgot to change IIDs

Like the previous patch but with changed IIDs. Sorry for the spam.

Comment 13

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 407232 [details] [diff] [review]
Forgot to change IIDs

Looks good.  Just a couple of gripes:

- IsScriptCreated is slightly confusingly named, since it could be interpreted as testing whether the parser has created a script, when really it tests whether the parser was created by a script.  I would prefer WasCreatedByScript (and thus also MarkAsNotCreatedByScript instead of MarkAsNotScriptCreated).

- Can we have some assurance that MarkAsNotScriptCreated is called only once?

- In nsHtml5TreeOpExecutor::Flush() and ::ForcedFlush(), I would prefer using mFlushing directly, instead of calling IsFlushing().  It would make the following test/assignment just a little more obviously symmetric:

-  if (IsFlushing()) {
+  if (mFlushing) {
     return;
   }
   mFlushing = PR_TRUE;

Comment 14

[Henri Sivonen (:hsivonen)]

Attachment: Address review comments

(In reply to comment #13)
> (From update of attachment 407232 [details] [diff] [review])
> Looks good.  Just a couple of gripes:

Thanks.

> - IsScriptCreated is slightly confusingly named, since it could be interpreted
> as testing whether the parser has created a script, when really it tests
> whether the parser was created by a script.  I would prefer WasCreatedByScript
> (and thus also MarkAsNotCreatedByScript instead of MarkAsNotScriptCreated).

"script-created parser" is HTML5 spec terminology. I wanted to align the method names (including RunScript vs. ExecuteScript) with the spec terminology.

> - Can we have some assurance that MarkAsNotScriptCreated is called only once?

Added an NS_PRECONDITION.

> - In nsHtml5TreeOpExecutor::Flush() and ::ForcedFlush(), I would prefer using
> mFlushing directly, instead of calling IsFlushing().  It would make the
> following test/assignment just a little more obviously symmetric:
> 
> -  if (IsFlushing()) {
> +  if (mFlushing) {
>      return;
>    }
>    mFlushing = PR_TRUE;

Fixed.

Comment 15

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

Comment on attachment 407504 [details] [diff] [review]
Address review comments

I admittedly have a very hard time following the flow of the code here. So relying on Bens review quite a bit.

Main thing is we really need tests for this stuff. Definitely don't land this without that.

Just some nits:

>diff --git a/content/base/public/nsIScriptElement.h b/content/base/public/nsIScriptElement.h
>+  void BeginEvaluating()
>+  {
>+    // Once the async attribute is supported, don't do this if this is an
>+    // async script.
>+    nsCOMPtr<nsIParser> parser = do_QueryReferent(mCreatorParser);
>+    if (parser) {
>+      parser->BeginEvaluatingParserInsertedScript();
>+    } else {
>+      mCreatorParser = nsnull;
>+    }
>+  }

There's not really a reason to set mCreatorParser to null. Same in EndEvaluating and GetCreatorParser. Or can you always set mCreatorParser to null in EndEvaluating (rather than just when the do_QR

>+  already_AddRefed<nsIParser> GetCreatorParser()
>+  {
>+    nsCOMPtr<nsIParser> parser = do_QueryReferent(mCreatorParser);
>+    if (parser) {
>+      nsIParser* retParser = parser;
>+      NS_ADDREF(retParser);
>+      return retParser;

Simply do |return parser.forget();|

sr=me

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: Mochitest

Comment 17

[Henri Sivonen (:hsivonen)]

Attachment: Address sr comment, move the call to IsInsertionPointDefined in nsHTMLDocument

(In reply to comment #15)
> Main thing is we really need tests for this stuff. Definitely don't land this
> without that.

Previously, I didn't know how to make HTTP responses deliberately stall on Mochitest. I've now learned how to test this sort of thing as a mochitest. I definitely should have learned that first and requested review only then. Sorry.

I found a flaw in the patch in the process and moved the call to IsInsertionPointDefined() in nsHTMLDocument.

> Just some nits:

> There's not really a reason to set mCreatorParser to null.

Fixed.

> Simply do |return parser.forget();|

Fixed.

Thank you.

Comment 18

[Henri Sivonen (:hsivonen)]

Comment on attachment 409938 [details] [diff] [review]
Address sr comment, move the call to IsInsertionPointDefined in nsHTMLDocument

This is like the previous patch except I addressed the sr comments and hoisted the IsInsertionPointDefined() call a few lines higher so that nsIParser::Terminate() gets called where needed.

Comment 19

[Henri Sivonen (:hsivonen)]

Attachment: Mochitest that runs the HTML5 parser--not the old parser

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #0)
> Need to make it comply to HTML5.
Could you add some links here to point to the place where this all is defined
in HTML5 draft.

Comment 21

[Henri Sivonen (:hsivonen)]

document.write() is defined to depend on the insertion point concept in
http://www.whatwg.org/specs/web-apps/current-work/#dom-document-write

The insertion point concept is defined in
http://www.whatwg.org/specs/web-apps/current-work/#insertion-point

Insertion point gets set to a non-undefined value in
http://www.whatwg.org/specs/web-apps/current-work/#parsing-main-incdata

The IsFlushing() part isn't spec-based currently. See
http://www.w3.org/Bugs/Public/show_bug.cgi?id=7926
http://lists.w3.org/Archives/Public/public-html/2009Oct/0424.html

Comment 22

[Henri Sivonen (:hsivonen)]

I think it's probably best to land the current patch on this bug regardless of the effects on async issues arising from bug 503481 having landed first and to deal with fallout in a follow-up bug to avoid breaking subsequent patches I have queued up after this one.

Comment 23

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

Comment on attachment 409938 [details] [diff] [review]
Address sr comment, move the call to IsInsertionPointDefined in nsHTMLDocument

Still would really like to see much more tests. Like nested document.writes, document.write from a deferred script, document.write from an async script (sorry, i'm sort of cheating there), document.write from both inline and external scripts.

Comment 24

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

Oh, also document.write from inside a sync-xhr event handler. I don't remember what the desired behavior is there, but it's something that's worth testing for no matter what.

Comment 25

[Henri Sivonen (:hsivonen)]

(In reply to comment #23)
> (From update of attachment 409938 [details] [diff] [review])

Thanks for the sr.

> Still would really like to see much more tests. Like nested document.writes,

I was thinking of dealing with those as part of bug 482913.

> document.write from a deferred script, 

defer scripts are borked with html5.enable regardless of this patch. (Covered by bug 515610.)

There's already a test for this in the tree (content/base/test/test_bug518104.html). Though to run it at the moment, one needs to patch html5.enable to true prior to running it. But that applies to pretty much everything. The only reason why I made this test toggle the pref is that the old parser doesn't pass this test.

> document.write from an async script
> (sorry, i'm sort of cheating there), 

I expect async scripts to be borked with html5.enable until bug 515610 is fixed.

> document.write from both inline and external scripts.

I expected the basic cases to get tested as side effects of other tests that use document.write().

(In reply to comment #24)
> Oh, also document.write from inside a sync-xhr event handler. I don't remember
> what the desired behavior is there, but it's something that's worth testing for
> no matter what.

I agree a test case for that would be good to have.

Comment 26

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

Ok, makes sense to let bug 482913 and bug 515610 deal with nested/async/defer.

(In reply to comment #25)
> > document.write from both inline and external scripts.
> 
> I expected the basic cases to get tested as side effects of other tests that
> use document.write().

I don't think we're testing document.write that heavily actually. So would be great with just some basic tests here.

> (In reply to comment #24)
> > Oh, also document.write from inside a sync-xhr event handler. I don't
> > remember what the desired behavior is there, but it's something that's
> > worth testing for no matter what.
> 
> I agree a test case for that would be good to have.

Isn't the intent for this bug to deal with this case according to spec? If so, testing for that here seems appropriate.

Comment 27

[Henri Sivonen (:hsivonen)]

(In reply to comment #26)
> Isn't the intent for this bug to deal with this case according to spec? If so,
> testing for that here seems appropriate.

The intent of this bug is to deal with this case per spec with the modification from 
http://lists.w3.org/Archives/Public/public-html/2009Oct/0424.html

The specs haven't yet figured out how sync XHR interacts with HTML5. Hence, 
http://www.w3.org/Bugs/Public/show_bug.cgi?id=7926

I'll write a test case.

Comment 28

[Henri Sivonen (:hsivonen)]

(In reply to comment #24)
> Oh, also document.write from inside a sync-xhr event handler.

Which event handler did you mean. It seems to me that sync XHR doesn't fire progress events.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

I guess Jonas meant readystatechanged events, which should fire only right before
entering the nested event loop and right after that.
But the patch to do that hasn't landed yet (or I backed out it). Bug 313646.

Comment 30

[Henri Sivonen (:hsivonen)]

(In reply to comment #29)
> I guess Jonas meant readystatechanged events, which should fire only right
> before
> entering the nested event loop and right after that.
> But the patch to do that hasn't landed yet (or I backed out it). Bug 313646.

OK. Thinking about this more, it's probably not particularly interesting to test *sync* XHR event handlers anyway. However, it would be interesting to test synchronous link element insertion events.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #30)
> However, it would be interesting to test
> synchronous link element insertion events.
What you mean here? DOMLinkAdded/Removed? Those are asynchronous.

Comment 32

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 409938 [details] [diff] [review]
Address sr comment, move the call to IsInsertionPointDefined in nsHTMLDocument

Looks good. I'll have to remember that .forget() trick.

Comment 33

[Henri Sivonen (:hsivonen)]

(In reply to comment #32)
> (From update of attachment 409938 [details] [diff] [review])
> Looks good. I'll have to remember that .forget() trick.

Thank you.

(In reply to comment #31)
> (In reply to comment #30)
> > However, it would be interesting to test
> > synchronous link element insertion events.
> What you mean here? DOMLinkAdded/Removed? Those are asynchronous.

Ah. I hadn't looked at the code but I was under the impression those were synchronous.

Comment 34

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 410219 [details] [diff] [review]
Mochitest that runs the HTML5 parser--not the old parser

r=me if you'll consider the following changes:

diff --git a/content/base/test/file_bug503473-frame.sjs b/content/base/test/file_bug503473-frame.sjs
--- a/content/base/test/file_bug503473-frame.sjs
+++ b/content/base/test/file_bug503473-frame.sjs
@@ -1,8 +1,9 @@
 function handleRequest(request, response) {
   response.processAsync();
   response.setStatusLine(request.httpVersion, 200, "OK");
   response.setHeader("Content-Type", "text/html; charset=utf-8", false);
+  response.setHeader("Cache-Control", "no-cache", false);
 
   response.write(
     '<!DOCTYPE html>' +
     '<div></div>' +
diff --git a/content/base/test/test_bug503473.html b/content/base/test/test_bug503473.html
--- a/content/base/test/test_bug503473.html
+++ b/content/base/test/test_bug503473.html
@@ -30,10 +30,8 @@ function done() {
   var divs = iframe.contentWindow.document.getElementsByTagName("div").length;
   is(divs, 0, "Div wasn't blown away.")
 
   netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
-  var prefs = Components.classes["@mozilla.org/preferences-service;1"]
-            .getService(Components.interfaces.nsIPrefBranch);
   prefs.setBoolPref("html5.enable", gOriginalHtml5Pref);
 
   SimpleTest.finish();
 }

Comment 35

[Henri Sivonen (:hsivonen)]

Thank you for the reviews.

Code landed as http://hg.mozilla.org/mozilla-central/rev/52a76741b65b
Test initially accidentally landed without addressing the review comments as http://hg.mozilla.org/mozilla-central/rev/8f141d7e1cb1
Test review comments addressed as http://hg.mozilla.org/mozilla-central/rev/c3f5eaa30cdf

Comment 36

[Henri Sivonen (:hsivonen)]

...and test disabled:
http://hg.mozilla.org/mozilla-central/rev/777a5293a53e