592656 - Use the most recent parser-inserted script as the parser key

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[Henri Sivonen (:hsivonen)]

document.write() insertion point tracking uses the current script from nsScriptLoader. It should instead use the current parser-inserted script which should get unset only after onload for the script has fired.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Fix without tests

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Match Chromium nightlies / spec

Comment 3

[Henri Sivonen (:hsivonen)]

Nominating as a blocker, since this is a simple-to-fix HTML5 parsing correctness bug with fix in hand.

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Deal with merge conflicts from bug 587931

I'll rerequest review once this has been through the tryserver

Comment 5

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

Can you give a simple example of testcase which changes behavior with this fix?

Comment 6

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

The changes generally look good, I'd just like to understand how we think things behave wrongly now, and how we want it to behave.

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to comment #5)
> Can you give a simple example of testcase which changes behavior with this fix?

Do you mean the test case in the patch is non-simple?

The WebKit test is:
http://trac.webkit.org/export/LATEST/trunk/LayoutTests/fast/tokenizer/write-on-load.html

(In reply to comment #6)
> The changes generally look good, I'd just like to understand how we think
> things behave wrongly now, and how we want it to behave.

We want writes from onload to behave as if they were the last writes from the script itself. Without this patch, the insertion point is computed for the previously "current" script so the writes would go somewhere different and non-sensical compared to what one would expect from the spec (if the writes weren't blowing away the document).

Part of the problem is that the current script is managed too close to the script evaluation. Another part of the problem is that we should be looking at the most current *parser-inserted* script anyway (to make things work right when a parser-inserted script inserts a script-inserted inline script that writes). The third part of the problem was that onload fired after writing had become prohibited again.

Comment 8

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

> (In reply to comment #6)
> > The changes generally look good, I'd just like to understand how we think
> > things behave wrongly now, and how we want it to behave.
> 
> We want writes from onload to behave as if they were the last writes from the
> script itself.

Why do we want that? As far as I can see <script onload="..."> doesn't work in IE at all so I doubt there is web contents that depends on it if it neither works in IE or Gecko.

IMHO we want to limit document.write() as much as we can.

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to comment #8)
> > (In reply to comment #6)
> > > The changes generally look good, I'd just like to understand how we think
> > > things behave wrongly now, and how we want it to behave.
> > 
> > We want writes from onload to behave as if they were the last writes from the
> > script itself.
> 
> Why do we want that?

That way, Gecko's behavior would become black-box equivalent to the conceptual model presented in the spec.

I don't know of any use cases for writing from onload, but writing the patch took less time than resisting the implications of what the spec says.

Furthermore, there was a need to write a patch anyway, because Gecko's behavior was bogus in other ways as well: In the case where a parser-inserted script inserts a script-inserted inline script that writes, without this patch, Gecko exhibits weird behavior that makes no sense until you dig deep into legacy code in Gecko. I don't really know how I could credibly argue for the behavior without this patch to be specced for that case. With this patch, Gecko would exhibit behavior that's coherent under the conceptual model described in the spec.

I'm assuming we want the behavior of document.write to be specced and we want Gecko to do what's specced.

Comment 10

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

(In reply to comment #9)
> (In reply to comment #8)
> > > (In reply to comment #6)
> > > > The changes generally look good, I'd just like to understand how we think
> > > > things behave wrongly now, and how we want it to behave.
> > > 
> > > We want writes from onload to behave as if they were the last writes from the
> > > script itself.
> > 
> > Why do we want that?
> 
> That way, Gecko's behavior would become black-box equivalent to the conceptual
> model presented in the spec.
> 
> I don't know of any use cases for writing from onload, but writing the patch
> took less time than resisting the implications of what the spec says.

Unless there is a reason to allow this I don't think we should. I don't think the implementation burden here is big enough that we need to take that into account.

Especially for event handlers it seems like a bad idea to allow document.write since event handlers often handle events sent from several different places, so this would both cause contents to be written to various places, and be a big blow-away-the-full-document hazard.

> Furthermore, there was a need to write a patch anyway, because Gecko's
> behavior was bogus in other ways as well: In the case where a parser-inserted
> script inserts a script-inserted inline script that writes, without this
> patch, Gecko exhibits weird behavior that makes no sense until you dig deep
> into legacy code in Gecko. I don't really know how I could credibly argue for
> the behavior without this patch to be specced for that case. With this patch,
> Gecko would exhibit behavior that's coherent under the conceptual model
> described in the spec.

The changes to this behavior sounds like a good idea. But doesn't sound like it mandates allowing document.write from onload?

> I'm assuming we want the behavior of document.write to be specced and we want
> Gecko to do what's specced.

Of course. I'm arguing the spec should be updated if it indeed requires document.write to be supported during the load handler execution.

Comment 11

[Henri Sivonen (:hsivonen)]

(In reply to comment #10)
> Unless there is a reason to allow this I don't think we should.

The main reasons are:
 1) Old browsers allowed it, so disallowing it seems riskier than allowing it.
 2) Allowing it follows as a logical consequence of how the spec (and, as I understand it, WebKit) manage the insertion point, so disallowing it would mean spec and WebKit changes.

> I don't think
> the implementation burden here is big enough that we need to take that into
> account.

If we want to change the spec here, we need to take the implementation burden in the WebKit context into account, too. Also, we should consider the risk of asking a spec change and Hixie changing the spec but in a subtly different way compared to how we asked the spec to be changed.

> Especially for event handlers it seems like a bad idea to allow document.write
> since event handlers often handle events sent from several different places, so
> this would both cause contents to be written to various places, and be a big
> blow-away-the-full-document hazard.

I hadn't considered the angle of a document.write-calling script load event handler function also being reused as an event handler for something else.

> But doesn't sound like it
> mandates allowing document.write from onload?

No, but it was simple to do both when changing this general area.

> > I'm assuming we want the behavior of document.write to be specced and we want
> > Gecko to do what's specced.
> 
> Of course. I'm arguing the spec should be updated if it indeed requires
> document.write to be supported during the load handler execution.

Could you work it out with Adam and Hixie in http://www.w3.org/Bugs/Public/show_bug.cgi?id=9984 ?

Comment 12

[Henri Sivonen (:hsivonen)]

sicking, what are the next steps here? Are you planning to push for a spec change? I think a spec change isn't worthwhile, and I'd prefer to land the patch here.

Comment 13

[Henri Sivonen (:hsivonen)]

Attachment: part 1 - Use the current parser-inserted script as the insertion point key

Splitting the patch into two parts per voice discussion with sicking.

Comment 14

[Henri Sivonen (:hsivonen)]

Attachment: part 2 - Define the insertion point before and undefine after the synchronous script execution events

Comment 15

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

Comment on attachment 486874 [details] [diff] [review]
part 1 - Use the current parser-inserted script as the insertion point key

Looks great

Comment 16

[Henri Sivonen (:hsivonen)]

Landed part 1:
http://hg.mozilla.org/mozilla-central/rev/5e3314152651

Comment 17

[Johnny Stenback (:jst)]

Henri, is part 2 ready for review?

Comment 18

[Henri Sivonen (:hsivonen)]

Comment on attachment 486875 [details] [diff] [review]
part 2 - Define the insertion point before and undefine after the synchronous script execution events

(In reply to comment #17)
> Henri, is part 2 ready for review?

The patch is ready for review. I didn't request review, because still in late October, sicking and I disagreed on whether Gecko should match the spec (what the patch does) or whether the spec should be changed to match unpatched Gecko.

I'd like to just land the patch and leave the spec as is. Since there's been no action on the spec change front, I'm requesting review to put this on sicking's radar again.

Comment 19

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

First patch has landed, so lets mark this fixed and deal with second patch in separate bug.

Comment 20

[Henri Sivonen (:hsivonen)]

The second bug is bug 619109.

Comment 21

[Henri Sivonen (:hsivonen)]

Comment on attachment 486875 [details] [diff] [review]
part 2 - Define the insertion point before and undefine after the synchronous script execution events

Attachment moved to bug 619109