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

Status: VERIFIED FIXED

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

Description

[Michał Lipiński]

Attachment: 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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Michał Lipiński]

I don't know if this will help to track, but workaround for this is to wrap location change with setTimeout(function() {}, 0);

Comment 3

[Alice0775 White]

Last good: a4d83f2fb3f2
First bad: 994b74a4a909
Triggered by: 
994b74a4a909	Henri Sivonen — Bug 715112 - Remove duplicate document.close() state tracking. r=smaug.

Comment 4

[Alex Keybl [:akeybl]]

Sending over to you, Henri, since you landed the regressing bug.

Comment 5

[Henri Sivonen (:hsivonen)]

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.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: 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.

Comment 7

[Henri Sivonen (:hsivonen)]

(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

Comment 8

[Henri Sivonen (:hsivonen)]

Filed bug 755679 as a follow-up.

Comment 9

[Henri Sivonen (:hsivonen)]

Even more stuff to fix here as a follow-up: bug 755682.

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: WIP that ignores a bogus spec requirement

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Fix with test

Still needs a try run.

Comment 12

[Henri Sivonen (:hsivonen)]

Attachment: Fix with test, now with accurate commit message

Comment 13

[Henri Sivonen (:hsivonen)]

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.

Comment 14

[Henri Sivonen (:hsivonen)]

Thanks for the r+. Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9901a0253c63

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9901a0253c63

Comment 16

[Henri Sivonen (:hsivonen)]

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.

Comment 17

[Alex Keybl [:akeybl]]

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.

Comment 18

[Henri Sivonen (:hsivonen)]

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

Setting earlier branches to wontfix per triage comment.

Comment 19

[Paul Silaghi, QA [:pauly]]

(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.

Comment 20

[Michał Lipiński]

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

Comment 21

[Alice0775 White]

(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)

Comment 22

[Paul Silaghi, QA [:pauly]]

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.