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

RESOLVED FIXED in Firefox 12

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(firefox11 unaffected, firefox12+ verified, firefox13+ verified, firefox14+ verified)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

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

Updated

6 years ago
Blocks: 738614
(Assignee)

Updated

6 years ago
Duplicate of this bug: 738614
(Assignee)

Updated

6 years ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Comment 3

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

Comment 4

6 years ago
Created attachment 609688 [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
(Assignee)

Comment 5

6 years ago
Setting tracking flags based on info in bug 738614 because the lack of this fix breaks a bank site.
status-firefox11: --- → unaffected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
tracking-firefox12: --- → ?
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
(Assignee)

Comment 6

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

Comment 7

6 years ago
Created 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

v2: Keep document.write throwing in XHTML even if the parser has been aborted.
Attachment #609688 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #609689 - Flags: review?(bugs)

Updated

6 years ago
tracking-firefox12: ? → +
tracking-firefox13: ? → +
tracking-firefox14: ? → +

Updated

6 years ago
Attachment #609689 - Flags: review?(bugs) → review+
(Assignee)

Comment 8

6 years ago
Thanks for the r+.

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

Comment 9

6 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
(Assignee)

Comment 10

6 years ago
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.
Attachment #609689 - Flags: approval-mozilla-beta?
Attachment #609689 - Flags: approval-mozilla-aurora?
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.
Attachment #609689 - Flags: approval-mozilla-beta?
Attachment #609689 - Flags: approval-mozilla-beta+
Attachment #609689 - Flags: approval-mozilla-aurora?
Attachment #609689 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 12

5 years ago
Thanks. Landed.

https://hg.mozilla.org/releases/mozilla-aurora/rev/d28f72430f65
https://hg.mozilla.org/releases/mozilla-beta/rev/a9c01eeccc44
status-firefox12: affected → fixed
status-firefox13: affected → fixed
status-firefox14: affected → fixed
Whiteboard: [qa+]

Comment 13

5 years ago
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).
status-firefox12: fixed → verified

Comment 14

5 years ago
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).
status-firefox13: fixed → verified

Comment 15

5 years ago
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

5 years ago
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
status-firefox14: fixed → verified
Whiteboard: [qa+]
You need to log in before you can comment on or make changes to this bug.