Calling window.close() should indicate failure with warning message

Calling window.close() should indicate failure with warning message

Calling window.close() has no effect on the main page as the calling script has not created the window.
In such case, a warning message should be added to the console stating the failure to honor window.close().

Firefox is showing the message "Scripts may not close windows that were not opened by script."
The same has been documented here https://developer.mozilla.org/en-US/docs/DOM/window.close

Also there is a bug in the DOMWindow::close() which closes the newly opened tab if window.close() is executed
from the console of inspector. This is incorrect as the scripts are not allowed to close the window which is not
created by it. Also if this is the only tab open, then the browser is also closed.

The existing condition checks whether the page is opened by DOM or the page has more than 1 backward/forward history 
items or the browser doesn't allow the script to close the window.
With a new tab opened and the window.close() being called from the inspector console, the second condition i.e. 
having only 1 item in backward/forward history evaluates to true. Its negated and hence the browser
honors the closing of window.

Instead, the if condition should first check whether the closing of window is allowed for the given script. Then it should 
check the rest of the conditions. This way closing of browser/tab is avoided.

BUG=170760
R=abarth, pfeldman, yurys

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=160700

[vivekg__, 2013-08-22 12:41:26]

Please take a look. Thank you!

[vivekg__, 2013-08-22 14:36:16]

Should we notify the user about the failure to honor the window.close() ?

https://codereview.chromium.org/22929032/diff/1/Source/core/page/DOMWindow.cpp
File Source/core/page/DOMWindow.cpp (right):

https://codereview.chromium.org/22929032/diff/1/Source/core/page/DOMWindow.cp...
Source/core/page/DOMWindow.cpp:869: if (!allowScriptsToCloseWindows ||
!page->openedByDOM() || page->backForward().count() > 1) {
I guess this behavior was added in webkit.org/b/22070 specifically for automated
testing of chrome. I think we should just retain this logic as is. I apologize
for not noticing this earlier.

[yurys, 2013-08-23 06:12:55]

https://codereview.chromium.org/22929032/diff/1/Source/core/page/DOMWindow.cpp
File Source/core/page/DOMWindow.cpp (right):

https://codereview.chromium.org/22929032/diff/1/Source/core/page/DOMWindow.cp...
Source/core/page/DOMWindow.cpp:869: if (!allowScriptsToCloseWindows ||
!page->openedByDOM() || page->backForward().count() > 1) {
On 2013/08/22 14:36:16, vivekg_ wrote:
> I guess this behavior was added in webkit.org/b/22070 specifically for
automated
> testing of chrome. I think we should just retain this logic as is. I apologize
> for not noticing this earlier.

Should we close this issue then?

[vivekg__, 2013-08-23 08:05:12]

On 2013/08/23 06:12:55, Yury Semikhatsky wrote:
> https://codereview.chromium.org/22929032/diff/1/Source/core/page/DOMWindow.cpp
> File Source/core/page/DOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/22929032/diff/1/Source/core/page/DOMWindow.cp...
> Source/core/page/DOMWindow.cpp:869: if (!allowScriptsToCloseWindows ||
> !page->openedByDOM() || page->backForward().count() > 1) {
> On 2013/08/22 14:36:16, vivekg_ wrote:
> > I guess this behavior was added in webkit.org/b/22070 specifically for
> automated
> > testing of chrome. I think we should just retain this logic as is. I
apologize
> > for not noticing this earlier.
> 
> Should we close this issue then?

At least we should display some message when its not going to close. I had
changed the if condition which can be as is though.

[yurys, 2013-08-23 08:19:06]

On 2013/08/23 08:05:12, vivekg_ wrote:
> On 2013/08/23 06:12:55, Yury Semikhatsky wrote:
> >
https://codereview.chromium.org/22929032/diff/1/Source/core/page/DOMWindow.cpp
> > File Source/core/page/DOMWindow.cpp (right):
> > 
> >
>
https://codereview.chromium.org/22929032/diff/1/Source/core/page/DOMWindow.cp...
> > Source/core/page/DOMWindow.cpp:869: if (!allowScriptsToCloseWindows ||
> > !page->openedByDOM() || page->backForward().count() > 1) {
> > On 2013/08/22 14:36:16, vivekg_ wrote:
> > > I guess this behavior was added in webkit.org/b/22070 specifically for
> > automated
> > > testing of chrome. I think we should just retain this logic as is. I
> apologize
> > > for not noticing this earlier.
> > 
> > Should we close this issue then?
> 
> At least we should display some message when its not going to close. I had
> changed the if condition which can be as is though.

Note that if you pass url of a page with the following content
<script>
window.close();
</script>
in command line then it will close the browser window immediately. However
loading that page by typing the url manually will have no effect so I'm not sure
what the point in (page->backForward().count() > 1) condition here.

Also you basically inverted the condition and now the page will only be closed
if (allowScriptsToCloseWindows && page->openedByDOM() &&
page->backForward().count() <= 1) which means in particular that it will never
be closed if allowScriptsToCloseWindows is false. Is that what you expected?

[vivekg__, 2013-08-23 08:55:16]

On 2013/08/23 08:19:06, Yury Semikhatsky wrote:
> On 2013/08/23 08:05:12, vivekg_ wrote:
> > On 2013/08/23 06:12:55, Yury Semikhatsky wrote:
> > >
> https://codereview.chromium.org/22929032/diff/1/Source/core/page/DOMWindow.cpp
> > > File Source/core/page/DOMWindow.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/22929032/diff/1/Source/core/page/DOMWindow.cp...
> > > Source/core/page/DOMWindow.cpp:869: if (!allowScriptsToCloseWindows ||
> > > !page->openedByDOM() || page->backForward().count() > 1) {
> > > On 2013/08/22 14:36:16, vivekg_ wrote:
> > > > I guess this behavior was added in webkit.org/b/22070 specifically for
> > > automated
> > > > testing of chrome. I think we should just retain this logic as is. I
> > apologize
> > > > for not noticing this earlier.
> > > 
> > > Should we close this issue then?
> > 
> > At least we should display some message when its not going to close. I had
> > changed the if condition which can be as is though.
> 
> Note that if you pass url of a page with the following content
> <script>
> window.close();
> </script>
> in command line then it will close the browser window immediately. However
> loading that page by typing the url manually will have no effect so I'm not
sure
> what the point in (page->backForward().count() > 1) condition here.
> 
> Also you basically inverted the condition and now the page will only be closed
> if (allowScriptsToCloseWindows && page->openedByDOM() &&
> page->backForward().count() <= 1) which means in particular that it will never
> be closed if allowScriptsToCloseWindows is false. Is that what you expected?

Oops, I guess my previous statement was stated incorrectly, apologies. I wanted
to convey that we should just report the warning message. I should revert the if
condition to one which is already there in the code.

[vivekg__, 2013-08-23 12:13:21]

This patch only adds the message. 

I had to remove the test cases which were added in the previous patch set due to
the window.close() is working during running of the layout tests. Is there a way
to configure it in such a way so it doen't allow closing of it?

On a side note, should the closing of a window be decided by the chrome client
(embedder) or some kind of policy decision inside the chrome client?

[pfeldman, 2013-10-24 13:51:11]

lgtm

[commit-bot: I haz the power, 2013-10-24 14:31:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/22929032/9001

[commit-bot: I haz the power, 2013-10-24 14:31:25]

Failed to apply patch for Source/core/page/DOMWindow.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: Source/core/page/DOMWindow.cpp
  |diff --git a/Source/core/page/DOMWindow.cpp b/Source/core/page/DOMWindow.cpp
  |index
8fc923eea56e0b368221612ce0eb5dbb51e26f4d..238a628a80e5c23f3fd24ac9e708e1b587dae468
100644
  |--- a/Source/core/page/DOMWindow.cpp
  |+++ b/Source/core/page/DOMWindow.cpp
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       Source/core/page/DOMWindow.cpp
Index: Source/core/page/DOMWindow.cpp
diff --git a/Source/core/page/DOMWindow.cpp b/Source/core/page/DOMWindow.cpp
index
8fc923eea56e0b368221612ce0eb5dbb51e26f4d..238a628a80e5c23f3fd24ac9e708e1b587dae468
100644
--- a/Source/core/page/DOMWindow.cpp
+++ b/Source/core/page/DOMWindow.cpp
@@ -866,8 +866,11 @@ void DOMWindow::close(ScriptExecutionContext* context)
     Settings* settings = m_frame->settings();
     bool allowScriptsToCloseWindows = settings &&
settings->allowScriptsToCloseWindows();
 
-    if (!(page->openedByDOM() || page->backForward().count() <= 1 ||
allowScriptsToCloseWindows))
+    if (!(page->openedByDOM() || page->backForward().count() <= 1 ||
allowScriptsToCloseWindows)) {
+        String message = "Scripts may close only the windows that were opened
by it.";
+        pageConsole()->addMessage(JSMessageSource, WarningMessageLevel,
message);
         return;
+    }
 
     if (!m_frame->loader()->shouldClose())
         return;

[eseidel, 2013-10-28 06:19:59]

lgtm

[commit-bot: I haz the power, 2013-10-28 06:20:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vivek.vg@samsung.com/22929032/42001

[commit-bot: I haz the power, 2013-10-28 07:12:41]

Change committed as 160700