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

RESOLVED FIXED

Status

()

Core
HTML: Parser
P2
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 final+)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Summary: Make document.write() inserted into the right place when invoked from script@onload or from a script-inserted inline script when a parser-inserted script is on the call stack → Make document.write() insert into the right place when invoked from script@onload or from a script-inserted inline script when a parser-inserted script is on the call stack
(Assignee)

Comment 1

7 years ago
Created attachment 471100 [details] [diff] [review]
Fix without tests
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Comment 2

7 years ago
Created attachment 471817 [details] [diff] [review]
Match Chromium nightlies / spec
Attachment #471100 - Attachment is obsolete: true
Attachment #471817 - Flags: review?(jonas)
(Assignee)

Comment 3

7 years ago
Nominating as a blocker, since this is a simple-to-fix HTML5 parsing correctness bug with fix in hand.
blocking2.0: --- → ?
Priority: -- → P2
(Assignee)

Updated

7 years ago
Depends on: 591981

Updated

7 years ago
blocking2.0: ? → final+
(Assignee)

Comment 4

7 years ago
Created attachment 475803 [details] [diff] [review]
Deal with merge conflicts from bug 587931

I'll rerequest review once this has been through the tryserver
Attachment #471817 - Attachment is obsolete: true
Attachment #471817 - Flags: review?(jonas)
(Assignee)

Updated

7 years ago
Attachment #475803 - Flags: review?(jonas)
Can you give a simple example of testcase which changes behavior with this fix?
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.
(Assignee)

Comment 7

7 years ago
(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.
> (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.
(Assignee)

Comment 9

7 years ago
(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.
(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.
(Assignee)

Comment 11

7 years ago
(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 ?
(Assignee)

Comment 12

7 years ago
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.
(Assignee)

Comment 13

7 years ago
Created attachment 486874 [details] [diff] [review]
part 1 - Use the current parser-inserted script as the insertion point key

Splitting the patch into two parts per voice discussion with sicking.
Attachment #475803 - Attachment is obsolete: true
Attachment #486874 - Flags: review?(jonas)
Attachment #475803 - Flags: review?(jonas)
(Assignee)

Comment 14

7 years ago
Created attachment 486875 [details] [diff] [review]
part 2 - Define the insertion point before and undefine after the synchronous script execution events
Comment on attachment 486874 [details] [diff] [review]
part 1 - Use the current parser-inserted script as the insertion point key

Looks great
Attachment #486874 - Flags: review?(jonas) → review+
(Assignee)

Comment 16

7 years ago
Landed part 1:
http://hg.mozilla.org/mozilla-central/rev/5e3314152651
Henri, is part 2 ready for review?
(Assignee)

Comment 18

7 years ago
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.
Attachment #486875 - Flags: review?(jonas)
(Assignee)

Updated

7 years ago
Whiteboard: [patch ready for review; disagreement over whether to fix Gecko or to change the spec]
First patch has landed, so lets mark this fixed and deal with second patch in separate bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

7 years ago
The second bug is bug 619109.
Summary: Make document.write() insert into the right place when invoked from script@onload or from a script-inserted inline script when a parser-inserted script is on the call stack → Use the most recent parser-inserted script as the parser key
Whiteboard: [patch ready for review; disagreement over whether to fix Gecko or to change the spec]
(Assignee)

Comment 21

7 years ago
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
Attachment #486875 - Flags: review?(jonas)
You need to log in before you can comment on or make changes to this bug.