Bug 34624 – window.close() should only work if opened via script.

• Status: ASSIGNED
• Component: WebCore Misc.

[Kavita Kanetkar]

Actual bug was: http://code.google.com/p/chromium/issues/detail?id=6773
Middle clicking opens a tab. If the new page opened in the tab has window.close() from script, the tab shuts.

Fix this in WebCore.

[Kavita Kanetkar]

Created attachment 48185 [details]
proposed fix

Proposed fix and a test.

[Darin Adler]

Comment on attachment 48185 [details]
proposed fix

This seems like a major change. I think that check has been <= 1 for the entire lifetime of WebKit.

Is this safe? Will this break some web pages that are currently working?

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 48185 [details])
> This seems like a major change. I think that check has been <= 1 for the entire
> lifetime of WebKit.
> 
> Is this safe? Will this break some web pages that are currently working?

I've been helping Kavita with this, and I should've done more research before this patch was posted.  Sorry about that :-(

I came across the following comment in my search for the origin of the "page->getHistoryLength() < 1" check:

http://trac.webkit.org/browser/trunk/WebCore/bindings/js/kjs_window.cpp?rev=17126#L1737

If the original reason for the history check was to match Netscape's behavior, and Firefox no longer seems to support that special case, does it make sense for us to continue to support it?  My gut reaction is that it makes the most sense to just remove the page->getHistoryLength() check entirely.

[Alexey Proskuryakov]

I also noticed that window.close() doesn't always work in Firefox now, not sure why that's better behavior.

Something that's not quite clear is why "opened via script" change is done by modifying code that checks history length. That seems fragile, if not wrong.

[Nate Chapin]

(In reply to comment #4)
> I also noticed that window.close() doesn't always work in Firefox now, not sure
> why that's better behavior.

I'm not sure which behavior is better, and I for one am happy to go either way with it.  But it is a little counter intuitive (to me anyway) that we have this exception to the general rule that window.close() only works when the window was opened by DOM.

> 
> Something that's not quite clear is why "opened via script" change is done by
> modifying code that checks history length. That seems fragile, if not wrong.

I was assuming that was just a case of copy/pasting an imprecise bug name from http://crbug.com/6773 :-)

[Alexey Proskuryakov]

When Firefox denies window.close(), it says "Scripts may not close windows that were not opened by script" in error log.

[Adam Barth]

Does this patch match the HTML5 specification for window.close() behavior?

[Nate Chapin]

(In reply to comment #7)
> Does this patch match the HTML5 specification for window.close() behavior?

AFAICT, the spec doesn't say anything about a special case for allowing window.close() when the history length is less than or equal to 1.  So I think the answer is Yes.

[Adam Barth]

Here's the operative text from HTML5:

[[
The close() method on Window objects should, if the corresponding browsing context A is an auxiliary browsing context that was created by a script (as opposed to by an action of the user), and if the browsing context of the script that invokes the method is allowed to navigate the browsing context A, close the browsing context A (and may discard it too).
]]

I'd rather change WebKit to match that text if possible.  If it's not possible, we should let the HTML5 community know so we have have the spec match reality.

[Kavita Kanetkar]

Created attachment 48345 [details]
Patch2

Thanks for the review. 

From what I understand the htlm5 spec says : allow window.close() if script opened it, do not allow otherwise.
The original bug was filed to reflect this. 

This patch removes the page->getHistoryLength() from the condition.
If it's not approved, I can close both the bugs (this and the original one in Chromium) and the behavior stays unchanged.

[Adam Barth]

Comment on attachment 48345 [details]
Patch2

I'm inclined to try this patch since it matches HTML5.  If we find that it breaks some pages, that's useful information to feed back into the HTML5 working group.

Let's hold off on committing this patch for a few days to give others a chance to comment.

[Darin Adler]

(In reply to comment #6)
> When Firefox denies window.close(), it says "Scripts may not close windows that
> were not opened by script" in error log.

That logging for developers seems like a good feature. We should have it too.

[Kavita Kanetkar]

Created attachment 48838 [details]
Patch3

Added error logging.

[Adam Barth]

Comment on attachment 48838 [details]
Patch3

I'm not sure that's the right way to log a message to the error console.  You might want to look at how the canAccess checks log to the error console.  Also, you might consider giving the developer more context about what's going one.  For example "Refused to close window because the window was not opened by a script."

[Kavita Kanetkar]

Created attachment 48917 [details]
patch4

Changed LOG_ERROR() to console()->addMessage() and changed the test expectation.

[Adam Barth]

Comment on attachment 48917 [details]
patch4

Ok.  It's too bad we don't have the proper line numbers when we log these kinds of errors.

Let's give Darin a chance to comment again before landing this patch.

[WebKit Commit Bot]

Comment on attachment 48917 [details]
patch4

Rejecting patch 48917 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 12238 test cases.
fast/events/window-click-close.html -> crashed

Exiting early after 1 failures. 6418 tests run.
252.28s total testing time

6417 test cases (99%) succeeded
1 test case (<1%) crashed
2 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/320332

[Eric Seidel (no email)]

Looks like a real crash related to this patch.  I can get you the crash log from the bot if you need.

[Eric Seidel (no email)]

Comment on attachment 48917 [details]
patch4

r- since it causes a test to crash.