717180 - Re-fix bug 98654 (Make document.write after window.location not blow away the document)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Henri Sivonen (:hsivonen)]

Bug 98654 has regressed somewhere along the way. Other browsers don't have the same bug, so we should probably re-fix the bug.

See http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-January/034293.html

Comment 1

[Henri Sivonen (:hsivonen)]

Potential approach to fixing: Add an "ignore document.open and .write" flag to the document. Set that flag when the document's parser is forcibly terminated (i.e. the parser doesn't end parsing naturally by reaching EOF).

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #0)
> See
> http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-January/034293.html

Hixie's reply: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2012-February/034869.html

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Add a flag to nsDocument for tracking the situation where we've nulled out mParser due to termination but HTML says we should have an aborted active parser whose insertion point is defined

Comment 5

[Henri Sivonen (:hsivonen)]

Setting tracking flags based on info in bug 738614 because the lack of this fix breaks a bank site.

Comment 6

[Henri Sivonen (:hsivonen)]

Note that bug 98654 had regressed earlier in the sense that the test case there had started failing. The exact cause of that regression is unknown. Bug 738614 shows that fixing bug 715112 regressed bug 98654 even more. The fix here fixes both http://bnc.ca/ and the ancient test case from bug 98654.

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Add a flag to nsDocument for tracking the situation where we've nulled out mParser due to termination but HTML says we should have an aborted active parser whose insertion point is defined, v2

v2: Keep document.write throwing in XHTML even if the parser has been aborted.

Comment 8

[Henri Sivonen (:hsivonen)]

Thanks for the r+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e82fc6f3bceb

Comment 9

[Ed Morley [:emorley]]

Please can you set the target milestone when landing on inbound, along the lines of http://blog.bonardo.net/2012/03/23/how-you-can-help-mozilla-inbound-sheriffs-when-pushing :-)

https://hg.mozilla.org/mozilla-central/rev/e82fc6f3bceb

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 609689 [details] [diff] [review]
Add a flag to nsDocument for tracking the situation where we've nulled out mParser due to termination but HTML says we should have an aborted active parser whose insertion point is defined, v2

[Approval Request Comment]
Regression caused by (bug #): Partially regressed by unknown landing and regressed even more by bug 715112
User impact if declined: Pages that set window.location first and then still call document.write won't work. An example is http://bnc.ca/ which is a bank, so the impact is bad for the customers of that bank.
Testing completed (on m-c, etc.): Has baken on m-c for a couple of days.
Risk to taking this patch (and alternatives if risky): Relatively low risk. Aligns us better with IE and WebKit. Very few changed lines of code. In theory could break something else but no breakage has been reported in a couple of days.
String changes made by this patch: None.

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 609689 [details] [diff] [review]
Add a flag to nsDocument for tracking the situation where we've nulled out mParser due to termination but HTML says we should have an aborted active parser whose insertion point is defined, v2

[Triage Comment]
Low-risk fix for a web regression in FF12. Approved for Aurora 13 and Beta 12 - please land asap.

Comment 12

[Henri Sivonen (:hsivonen)]

Thanks. Landed.

https://hg.mozilla.org/releases/mozilla-aurora/rev/d28f72430f65
https://hg.mozilla.org/releases/mozilla-beta/rev/a9c01eeccc44

Comment 13

[Ioana (away)]

The test in the patch is passing on all OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=10983981&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=10985014&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=10985000&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=10987331&full=1&branch=mozilla-beta

Also verified as fixed with the test case from bug 98654 on Firefox 12.0 beta 6 (BuildID: 20120417165043).

Comment 14

[Ioana (away)]

The test in the patch (test_bug717180.html) is passing on all the OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=11207568&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=11209113&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=11212748&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=11212730&full=1&branch=mozilla-beta

I have also verified this fix with the test case from bug 98654 on Firefox 13.0 beta 1 (BuildID: 20120425123149).

Comment 15

[Ioana (away)]

The test in the patch (test_bug717180.html) is passing on all the OSs for Firefox 14.0 beta too:
https://tbpl.mozilla.org/php/getParsedLog.php?id=12407552&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12407322&full=1&branch=mozilla-beta
https://tbpl.mozilla.org/php/getParsedLog.php?id=12410323&full=1&branch=mozilla-beta

Comment 16

[Ioana (away)]

Verified this fix with the test case from bug 98654:
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20100101 Firefox/14.0 BuildID: 20120605113340