Last Comment Bug 753278 - document.close and document.open should be no-ops after window.location
: document.close and document.open should be no-ops after window.location
Status: VERIFIED FIXED
[qa+:paul.silaghi]
: regression, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: x86 Windows XP
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Henri Sivonen (:hsivonen)
:
Mentors:
http://mylith.com/mozilla/753278/
Depends on:
Blocks: 715112
  Show dependency treegraph
 
Reported: 2012-05-09 04:29 PDT by Michał Lipiński
Modified: 2012-06-22 05:42 PDT (History)
8 users (show)
hsivonen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
wontfix
+
verified
+
verified


Attachments
bug_location.tgz (718 bytes, application/x-tar-gz)
2012-05-09 04:29 PDT, Michał Lipiński
no flags Details
WIP; fixes the test case (3.21 KB, patch)
2012-05-16 01:57 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
WIP that ignores a bogus spec requirement (1.62 KB, patch)
2012-05-16 04:32 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Fix with test (3.74 KB, patch)
2012-05-18 02:00 PDT, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review
Fix with test, now with accurate commit message (3.72 KB, patch)
2012-05-18 03:12 PDT, Henri Sivonen (:hsivonen)
bugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta-
Details | Diff | Splinter Review

Description Michał Lipiński 2012-05-09 04:29:12 PDT
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.
Comment 1 Boris Zbarsky [:bz] 2012-05-09 09:07:04 PDT
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 2012-05-09 10:19:41 PDT
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 2012-05-09 16:21:25 PDT
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] 2012-05-09 16:35:25 PDT
Sending over to you, Henri, since you landed the regressing bug.
Comment 5 Henri Sivonen (:hsivonen) 2012-05-16 00:24:13 PDT
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) 2012-05-16 01:57:45 PDT
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.
Comment 7 Henri Sivonen (:hsivonen) 2012-05-16 04:20:40 PDT
(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) 2012-05-16 04:23:27 PDT
Filed bug 755679 as a follow-up.
Comment 9 Henri Sivonen (:hsivonen) 2012-05-16 04:29:59 PDT
Even more stuff to fix here as a follow-up: bug 755682.
Comment 10 Henri Sivonen (:hsivonen) 2012-05-16 04:32:59 PDT
Created attachment 624340 [details] [diff] [review]
WIP that ignores a bogus spec requirement
Comment 11 Henri Sivonen (:hsivonen) 2012-05-18 02:00:50 PDT
Created attachment 625038 [details] [diff] [review]
Fix with test

Still needs a try run.
Comment 12 Henri Sivonen (:hsivonen) 2012-05-18 03:12:44 PDT
Created attachment 625050 [details] [diff] [review]
Fix with test, now with accurate commit message
Comment 13 Henri Sivonen (:hsivonen) 2012-05-18 04:09:58 PDT
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) 2012-05-18 05:08:03 PDT
Thanks for the r+. Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9901a0253c63
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-05-18 18:06:53 PDT
https://hg.mozilla.org/mozilla-central/rev/9901a0253c63
Comment 16 Henri Sivonen (:hsivonen) 2012-05-21 06:58:18 PDT
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] 2012-05-21 15:25:07 PDT
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) 2012-05-22 03:31:58 PDT
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] 2012-06-21 07:21:33 PDT
(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 2012-06-21 07:25:21 PDT
Nightly 16.0a1 (2012-06-13) works fine for me
Nightly 16.0a1 (2012-06-21) also works as expected
Comment 21 Alice0775 White 2012-06-21 07:29:58 PDT
(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] 2012-06-22 05:42:32 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.