document.close and document.open should be no-ops after window.location

VERIFIED FIXED in Firefox 14

Status

()

Core
DOM
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Michał Lipiński, Assigned: hsivonen)

Tracking

({regression, testcase})

12 Branch
mozilla15
x86
Windows XP
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12 wontfix, firefox13+ wontfix, firefox14+ verified, firefox15+ verified)

Details

(Whiteboard: [qa+:paul.silaghi], URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 622342 [details]
bug_location.tgz

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Steps to reproduce:

Hello,

After update to 12 I have found a bug?

Change of window.location in frame doesn't get expected result, there is no any error so I can't provide more information so there is only testcase.

Tested against:

OK:
Mozilla/5.0 (Windows NT 5.1; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Mozilla/5.0 (Windows NT 5.1; rv:6.0.2) Gecko/20100101 Firefox/6.0.2
Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0
Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 5.1; Trident/4.0; .NET CLR 2.0.50727; .NET CLR 3.0.4506.2152; .NET CLR 3.5.30729; .NET4.0C)

Mozilla/5.0 (Windows NT 5.1) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.41 Safari/536.5

FAIL:
Mozilla/5.0 (Windows NT 5.1; rv:12.0) Gecko/20100101 Firefox/12.0


Actual results:

Browser shows only blank page (frame).


Expected results:

Frame should contain "hello" message.
(Reporter)

Updated

5 years ago
Keywords: testcase
(Reporter)

Updated

5 years ago

Updated

5 years ago
Attachment #622342 - Attachment mime type: text/plain → application/x-tar-gz
OK.  So what this page is doing is loading a subframe which runs a script which does the following:

1)  Set location on that subframe
2)  Call close() on the current document in that subframe, then call open() on that same
    document, write some stuff to it, and close it again.

in that order.  It seems to expect the location set to take effect.

The behavior change on our side happened in this range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5c2bc94d359c&tochange=e46cca506613

Presumably a regression from bug 715739 or bug 715112, though there are other bugs in that range that might be relevant too.

Henri, Olli, can you take a look, please?

Requesting tracking for the regression.
tracking-firefox13: --- → ?
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
Component: General → DOM
Keywords: regression
QA Contact: general → general
(Reporter)

Comment 2

5 years ago
I don't know if this will help to track, but workaround for this is to wrap location change with setTimeout(function() {}, 0);
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

5 years ago
Last good: a4d83f2fb3f2
First bad: 994b74a4a909
Triggered by: 
994b74a4a909	Henri Sivonen — Bug 715112 - Remove duplicate document.close() state tracking. r=smaug.
Blocks: 715112
Component: DOM → General

Comment 4

5 years ago
Sending over to you, Henri, since you landed the regressing bug.
Assignee: nobody → hsivonen
tracking-firefox13: ? → +
tracking-firefox14: ? → +
tracking-firefox15: ? → +

Updated

5 years ago
Assignee: hsivonen → nobody
Component: General → DOM

Updated

5 years ago
Assignee: nobody → hsivonen
(Assignee)

Comment 5

5 years ago
Following the letter of the spec seems to require adding yet another flag to documents to record whether the aborted parser was a script-created parser. Sigh.
Status: NEW → ASSIGNED
Summary: Unable to change window.location during document modification → document.close and document.open should be no-ops after window.location
(Assignee)

Comment 6

5 years ago
Created attachment 624326 [details] [diff] [review]
WIP; fixes the test case

While looking at the code, I noticed that the code doesn't quite match the spec in other cases, either. Need to investigate a bit.
(Assignee)

Comment 7

5 years ago
(In reply to Henri Sivonen (:hsivonen) from comment #5)
> Following the letter of the spec seems to require adding yet another flag to
> documents to record whether the aborted parser was a script-created parser.

Or maybe not! See https://www.w3.org/Bugs/Public/show_bug.cgi?id=17077
(Assignee)

Comment 8

5 years ago
Filed bug 755679 as a follow-up.
(Assignee)

Comment 9

5 years ago
Even more stuff to fix here as a follow-up: bug 755682.
(Assignee)

Comment 10

5 years ago
Created attachment 624340 [details] [diff] [review]
WIP that ignores a bogus spec requirement
Attachment #624326 - Attachment is obsolete: true

Updated

5 years ago
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
(Assignee)

Comment 11

5 years ago
Created attachment 625038 [details] [diff] [review]
Fix with test

Still needs a try run.
Attachment #624340 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
Created attachment 625050 [details] [diff] [review]
Fix with test, now with accurate commit message
Attachment #625038 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Comment on attachment 625050 [details] [diff] [review]
Fix with test, now with accurate commit message

Note that document.close() is already a no-op when the parser has been aborted, since it is a no-op when mParser is null.
Attachment #625050 - Flags: review?(bugs)

Updated

5 years ago
Attachment #625050 - Flags: review?(bugs) → review+
(Assignee)

Comment 14

5 years ago
Thanks for the r+. Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9901a0253c63
Flags: in-testsuite+
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/9901a0253c63
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox15: affected → fixed
(Assignee)

Comment 16

5 years ago
Comment on attachment 625050 [details] [diff] [review]
Fix with test, now with accurate commit message

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 715112
User impact if declined: Wrong page (likely blank page) shown to the user in some rare cases involving non-sensical scripting.
Testing completed (on m-c, etc.): Has baked on on central over the weekend without obvious complaints.
Risk to taking this patch (and alternatives if risky): The risk is breaking some weird document.open() stuff in other cases, but considering what other browser do and what Firefox 3.6 used to do, this patch is about as low risk as Web compat fixes in this area can be. The alternative is shipping 13 and 14 with the bug and letting the fix ride the 15 train. The patch that caused the regression shipped in Firefox 12 and has been built upon, so backing it out would be notable worse than taking this patch.
String or UUID changes made by this patch: None.
Attachment #625050 - Flags: approval-mozilla-beta?
Attachment #625050 - Flags: approval-mozilla-aurora?
Comment on attachment 625050 [details] [diff] [review]
Fix with test, now with accurate commit message

[Triage Comment]
Approving for Aurora, but denying for Beta given the risk evaluation. Since we don't have any dupes of this bug, believe it to be rare, and didn't consider chemspilling FF12, we'll let this ride FF14 and up.
Attachment #625050 - Flags: approval-mozilla-beta?
Attachment #625050 - Flags: approval-mozilla-beta-
Attachment #625050 - Flags: approval-mozilla-aurora?
Attachment #625050 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 18

5 years ago
Thanks. Landed on Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/349c6e3f5943

Setting earlier branches to wontfix per triage comment.
status-firefox12: affected → wontfix
status-firefox13: affected → wontfix
status-firefox14: affected → fixed
(In reply to Michael Lipinski from comment #0)
> Created attachment 622342 [details]
> bug_location.tgz

> Actual results:
> 
> Browser shows only blank page (frame).
> 
> 
> Expected results:
> 
> Frame should contain "hello" message.

I don't see the "hello" message loading the test case. Tested on Nightly 2012-06-20.
(Reporter)

Comment 20

5 years ago
Nightly 16.0a1 (2012-06-13) works fine for me
Nightly 16.0a1 (2012-06-21) also works as expected

Comment 21

5 years ago
(In reply to Paul Silaghi [QA] from comment #19)
> (In reply to Michael Lipinski from comment #0)
> > Created attachment 622342 [details]
> > bug_location.tgz
> 
> > Actual results:
> > 
> > Browser shows only blank page (frame).
> > 
> > 
> > Expected results:
> > 
> > Frame should contain "hello" message.
> 
> I don't see the "hello" message loading the test case. Tested on Nightly
> 2012-06-20.

you should open  testcase from http: protocol.(I.e, you need to put the testcase inside httpd server)
Sorry, I missed that. Thanks Alice.
Everything looks OK on:
Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20100101 Firefox/14.0b8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20100101 Firefox/14.0b8
Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20120621 Firefox/15.0a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120621 Firefox/15.0a2
Verified fixed.
Status: RESOLVED → VERIFIED
status-firefox14: fixed → verified
status-firefox15: fixed → verified
Whiteboard: [qa+:paul.silaghi]
You need to log in before you can comment on or make changes to this bug.