Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Description • 15 years ago

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).

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 1 • 15 years ago

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

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 2 • 15 years ago

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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 3 • 15 years ago

Need to test this a bit before asking for r.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 4 • 15 years ago

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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 5 • 15 years ago

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
 ?

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 6 • 15 years ago

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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 7 • 15 years ago

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

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 8 • 15 years ago

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. :-(

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 9 • 15 years ago

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

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 12 • 15 years ago

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

	Ben Newman (:bnewman) (:benjamn)
	Comment 13 • 15 years ago

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;

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 14 • 15 years ago

(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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 15 • 15 years ago

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

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 17 • 15 years ago

(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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 18 • 15 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 20 • 15 years ago

(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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 21 • 15 years ago

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

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 22 • 15 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 23 • 15 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 24 • 15 years ago

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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 25 • 15 years ago

(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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 26 • 15 years ago

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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 27 • 15 years ago

(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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 28 • 15 years ago

(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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 29 • 15 years ago

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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 30 • 15 years ago

(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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 31 • 15 years ago

(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.

	Ben Newman (:bnewman) (:benjamn)
	Comment 32 • 15 years ago

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.

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 33 • 15 years ago

(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.

	Ben Newman (:bnewman) (:benjamn)
	Comment 34 • 15 years ago

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();
 }

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 35 • 15 years ago

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

	Henri Sivonen (:hsivonen) (away from Bugzilla until 2023-09-11) Assignee
	Comment 36 • 15 years ago

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