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. Chrome review URL: https://codereview.chromium.org/22929032/ Patch follows.
Created attachment 209354 [details] Patch
Comment on attachment 209354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209354&action=review > Source/WebCore/ChangeLog:13 > + 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 Why? Is this behavior mandated by some specification? Is this intended as a feature for Inspector? If so, we might want to come up with a message that signify the fact we allow script to close the window in some cases. Most importantly, we can't copy & paste Firefox's error message without copying the copyright. That does for things on MDN as well. > Source/WebCore/page/DOMWindow.cpp:990 > bool allowScriptsToCloseWindows = m_frame->settings().allowScriptsToCloseWindows(); > > - if (!(page->openedByDOM() || page->backForward()->count() <= 1 || allowScriptsToCloseWindows)) > + if (!allowScriptsToCloseWindows || !page->openedByDOM() || page->backForward()->count() > 1) { We're not disallowing a window to be closed even if it's opened by DOM if the backforward count is more than 1. Is that intended? It also appears to be that allowScriptsToCloseWindows used to allow any window to be closed but now we only allow a window to be closed when it's opened by DOM. That seems like a big change in its semantics. According to webkit.org/b/22070, this feature was added for Chromium's automated testing. Are you sure most ports set this value intentionally? > LayoutTests/fast/loader/window-close-on-load-expected.txt:2 > +layer at (0,0) size 800x600 Why is this not a dumpAsText test? > LayoutTests/inspector/console/console-eval-window-close.html:1 > +<html> Missing DOCTYPE.
Comment on attachment 209354 [details] Patch Attachment 209354 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1550085 New failing tests: http/tests/misc/iframe-reparenting-id-collision.html http/tests/cookies/third-party-cookie-relaxing.html fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html http/tests/cache/history-only-cached-subresource-loads.html editing/pasteboard/copy-image-with-alt-text.html http/tests/inspector-protocol/access-inspected-object.html fast/dom/Window/mozilla-focus-blur.html fast/dom/Window/setting-properties-on-closed-window.html http/tests/misc/slow-preload-cancel.html fast/dom/Window/open-zero-size-as-default.html fast/dom/Document/early-document-access.html http/tests/inspector/inspect-element.html
Created attachment 209361 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
(In reply to comment #2) Thank you Ryosuke for the comments. > (From update of attachment 209354 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209354&action=review > > > Source/WebCore/ChangeLog:13 > > + 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 > > Why? Is this behavior mandated by some specification? Is this intended as a feature for Inspector? > If so, we might want to come up with a message that signify the fact we allow script to close the window in some cases. > I agree, its not part of any specification. I also think the message can be more intuitive depicting the correct scenario. Based on the DOM history, the inspector console behaves differently i.e. in one scenario it allows the window to be closed and not otherwise. Thought it would confuse the developers about the usage of window.close() and hence the patch. > Most importantly, we can't copy & paste Firefox's error message without copying the copyright. That does for things on MDN as well. > Oh yeah, missed this one! Will correct if we are going to use the message. Thanks for pointing out! > > Source/WebCore/page/DOMWindow.cpp:990 > > bool allowScriptsToCloseWindows = m_frame->settings().allowScriptsToCloseWindows(); > > > > - if (!(page->openedByDOM() || page->backForward()->count() <= 1 || allowScriptsToCloseWindows)) > > + if (!allowScriptsToCloseWindows || !page->openedByDOM() || page->backForward()->count() > 1) { > > We're not disallowing a window to be closed even if it's opened by DOM if the backforward count is more than 1. Is that intended? > It also appears to be that allowScriptsToCloseWindows used to allow any window to be closed but now we only allow a window to be closed when it's opened by DOM. > That seems like a big change in its semantics. According to webkit.org/b/22070, this feature was added for Chromium's automated testing. > Are you sure most ports set this value intentionally? > Oh, then it might be still required for the automation testing, we can leave this part as is. I wasn't aware of it, sorry! > > LayoutTests/fast/loader/window-close-on-load-expected.txt:2 > > +layer at (0,0) size 800x600 > > Why is this not a dumpAsText test? > Will change to dumpAsText, thanks! > > LayoutTests/inspector/console/console-eval-window-close.html:1 > > +<html> > > Missing DOCTYPE. Will add it.
Comment on attachment 209354 [details] Patch Attachment 209354 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1494467 New failing tests: fast/dom/Window/mozilla-focus-blur.html http/tests/misc/iframe-reparenting-id-collision.html fast/dom/location-new-window-no-crash.html fast/dom/Window/window-early-properties.html fast/forms/multiple-form-submission-protection-mouse.html fast/dom/Window/setting-properties-on-closed-window.html http/tests/misc/slow-preload-cancel.html fast/dom/Document/early-document-access.html http/tests/inspector/inspect-element.html http/tests/appcache/crash-when-navigating-away-then-back.html fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html http/tests/cache/history-only-cached-subresource-loads.html fast/loader/stateobjects/replacestate-in-iframe.html http/tests/cookies/third-party-cookie-relaxing.html http/tests/cache/history-only-cached-subresource-loads-max-age-https.html fast/events/page-visibility-iframe-move-test.html fast/history/history_reload.html http/tests/misc/set-window-opener-to-null.html fast/events/open-window-from-another-frame.html fast/dom/Window/open-zero-size-as-default.html fast/dom/Window/new-window-opener.html fast/dom/Geolocation/window-close-crash.html fast/harness/show-modal-dialog.html fast/history/window-open.html http/tests/inspector-protocol/access-inspected-object.html fast/dom/open-and-close-by-DOM.html fast/dom/Window/Location/set-location-after-close.html fast/loader/stateobjects/popstate-fires-with-page-cache.html fast/files/domurl-script-execution-context-crash.html http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html
Created attachment 209363 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.4
Comment on attachment 209354 [details] Patch Attachment 209354 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1424098 New failing tests: http/tests/inspector-protocol/access-inspected-object.html http/tests/appcache/crash-when-navigating-away-then-back.html fast/files/domurl-script-execution-context-crash.html fast/harness/show-modal-dialog.html fast/dom/open-and-close-by-DOM.html fast/dom/Window/new-window-opener.html http/tests/cookies/third-party-cookie-relaxing.html fast/dom/HTMLBaseElement/href-attribute-resolves-with-respect-to-document.html fast/dom/Document/early-document-access.html fast/dom/Window/Location/set-location-after-close.html fast/dom/Window/mozilla-focus-blur.html fast/forms/multiple-form-submission-protection-mouse.html fast/dom/Geolocation/window-close-crash.html http/tests/cache/history-only-cached-subresource-loads-max-age-https.html fast/dom/Window/setting-properties-on-closed-window.html http/tests/cache/history-only-cached-subresource-loads.html fast/dom/Window/open-zero-size-as-default.html fast/dom/location-new-window-no-crash.html http/tests/inspector/inspect-element.html
Created attachment 209366 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.4
Yes, it would be nice to have a helpful message in Inspector. It's quite confusing when basic calls like close() fail silently. > Also there is a bug in the DOMWindow::close() Could you split this into a separate bug? I'm not sure if scripts in console should be subject to security checks, and it's best to have a focused discussion.
(In reply to comment #10) > Yes, it would be nice to have a helpful message in Inspector. It's quite confusing when basic calls like close() fail silently. > > > Also there is a bug in the DOMWindow::close() > > Could you split this into a separate bug? I'm not sure if scripts in console should be subject to security checks, and it's best to have a focused discussion. Thank you @ap for the feedback. I will create a different bug for these two issues.
Created attachment 209639 [details] Patch
(In reply to comment #12) > Created an attachment (id=209639) [details] > Patch In this patch I am unable to get the tests working with the warning message as while running these tests, the window.close() works. How do I work around it? Adding some support functions in WebCore::Internals for disabling the window close?
If the test opens a child window, will it be able to close itself?
Comment on attachment 209639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209639&action=review > Source/WebCore/page/DOMWindow.cpp:991 > + String message = "Script may close only the windows those were opened by it."; Should wrap this literal in ASCIILiteral() for more efficient code. Not grammatically correct. I think you mean to say: Scripts may close only windows that were opened by scripts. But am not sure if this message matches the style of other console messages, nor is it precise. I’d welcome some discussion of the best wording for the message. Also seems to me there is no reason to put message in a local variable; I suggest just putting it right on the line with addMessage.
(In reply to comment #14) > If the test opens a child window, will it be able to close itself? I need to try this. Will try and update on this.
(In reply to comment #15) Thank you Darin for the feedback. > (From update of attachment 209639 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209639&action=review > > > Source/WebCore/page/DOMWindow.cpp:991 > > + String message = "Script may close only the windows those were opened by it."; > > Should wrap this literal in ASCIILiteral() for more efficient code. Not grammatically correct. I think you mean to say: > > Scripts may close only windows that were opened by scripts. > Sounds good to me. > But am not sure if this message matches the style of other console messages, nor is it precise. I’d welcome some discussion of the best wording for the message. > > Also seems to me there is no reason to put message in a local variable; I suggest just putting it right on the line with addMessage. Yeah sure, will change it accordingly, thanks!
Created attachment 209758 [details] Patch
Comment on attachment 209758 [details] Patch Seems OK. Maybe someone can come up with an even better version of the message. I think people who work on the inspector might have some thoughts on that.
Comment on attachment 209758 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209758&action=review > Source/WebCore/page/DOMWindow.cpp:991 > + pageConsole()->addMessage(JSMessageSource, WarningMessageLevel, ASCIILiteral("Scripts may close only windows that were opened by scripts.")); Maybe: "Can't close window since it wasn't opened by JavaScript."
(In reply to comment #20) > Maybe: "Can't close window since it wasn't opened by JavaScript." That does sound like better wording. What is the wording like for other console messages? I’d love to look at them all together.
Here are the one's easily found with grep: "window.webkitStorageInfo is deprecated. Use navigator.webkitTemporaryStorage or navigator.webkitPersistentStorage instead." "AudioBufferSourceNode 'looping' attribute is deprecated. Use 'loop' instead." "AudioBufferSourceNode 'looping' attribute is deprecated. Use 'loop' instead." "'soundfield' panning model not implemented." "Invalid url for WebSocket " + m_url.stringCenterEllipsizedToLength() "Wrong url scheme for WebSocket " + m_url.stringCenterEllipsizedToLength() "URL has fragment component " + m_url.stringCenterEllipsizedToLength() "WebSocket port " + String::number(m_url.port()) + " blocked" "Wrong protocol for WebSocket '" + encodeProtocolString(protocols[i]) + "'" "WebSocket protocols contain duplicates: '" + encodeProtocolString(protocols[i]) + "'" "Websocket message contains invalid character(s)." "WebSocket close message is too long." "'" + binaryType + "' is not a valid value for binaryType; binaryType remains unchanged." "WebSocket connection to '" + m_handshake->url().stringCenterEllipsizedToLength() + "' failed: " + reason "Blocked script execution in '" + m_frame->document()->url().stringCenterEllipsizedToLength() + "' because the document's frame is sandboxed and the 'allow-scripts' permission is not set." "Refused to execute script from '" + m_cachedScript->url().stringCenterEllipsizedToLength() + "' because its MIME type ('" + m_cachedScript->mimeType() + "') is not executable, and strict MIME type checking is enabled." "Blocked autofocusing on a form control because the form's frame is sandboxed and the 'allow-scripts' permission is not set." "Error while parsing the 'sandbox' attribute: " + invalidTokens "'HTMLMediaElement.webkitGenerateKeyRequest()' is deprecated. Use 'MediaKeys.createSession()' instead." "'HTMLMediaElement.webkitAddKey()' is deprecated. Use 'MediaKeySession.update()' instead." "Error parsing header X-XSS-Protection: " + headerValue + ": " + errorDetails + " at character position " + String::format("%u", errorPosition) + ". The default protections will be applied." "Application Cache download process was aborted." "Application Cache update failed, because " + m_currentHandle->firstRequest().url().stringCenterEllipsizedToLength() + "Application Cache update failed, because size quota was exceeded." "Application Cache update failed, because " + url.stringCenterEllipsizedToLength() + " could not be fetched." "Application Cache manifest could not be fetched." "Application Cache manifest could not be fetched, because a redirection was attempted." "Application Cache manifest could not be fetched because of an unexpected 304 Not Modified server response." "Application Cache manifest could not be parsed. Does it start with CACHE MANIFEST?" "Application Cache update failed, because size quota was exceeded." "Blocked form submission to '" + submission->action().stringCenterEllipsizedToLength() + "' because the form's frame is sandboxed and the 'allow-forms' permission is not set." "Not allowed to load local resource: " + url "Blocked attempt to show multiple beforeunload confirmation dialogs for the same navigation." "Blocked attempt to show beforeunload confirmation dialog on behalf of a frame with different security origin. Protocols, domains, and ports must match." "Multiple 'X-Frame-Options' headers with conflicting values ('" + content + "') encountered when loading '" + url.stringCenterEllipsizedToLength() + "'. Falling back to 'DENY'." "Invalid 'X-Frame-Options' header encountered when loading '" + url.stringCenterEllipsizedToLength() + "': '" + content + "' is not a recognized directive. The header will be ignored." "Blocked opening '" + request.resourceRequest().url().stringCenterEllipsizedToLength() + "' in a new window because the request was made in a sandboxed frame whose 'allow-popups' permission is not set." "Blocked pointer lock on an element because the element's frame is sandboxed and the 'allow-pointer-lock' permission is not set." "XPathNSResolver does not have a lookupNamespaceURI method." These files also add console messages, but pass the message in as a variable (so I couldn't easily extract the messages): Modules/webdatabase/DatabaseBase.cpp Modules/webdatabase/DatabaseManager.cpp Modules/websockets/WebSocketChannel.cpp bindings/js/JSAudioContextCustom.cpp css/MediaList.cpp dom/Document.cpp dom/Document.h dom/ScriptElement.cpp dom/ScriptExecutionContext.cpp dom/ScriptExecutionContext.h dom/ViewportArguments.cpp html/canvas/CanvasRenderingContext2D.cpp html/canvas/WebGLRenderingContext.cpp html/HTMLFormElement.cpp html/parser/XSSAuditorDelegate.cpp inspector/InspectorConsoleAgent.cpp inspector/InspectorConsoleAgent.h loader/cache/CachedResourceLoader.cpp loader/DocumentLoader.cpp loader/ImageLoader.cpp loader/MixedContentChecker.cpp loader/TextTrackLoader.cpp page/ContentSecurityPolicy.cpp page/EventSource.cpp svg/SVGDocumentExtensions.cpp workers/DefaultSharedWorkerRepository.cpp workers/WorkerGlobalScope.cpp workers/WorkerGlobalScope.h workers/WorkerMessagingProxy.cpp xml/XMLHttpRequest.cpp
Created attachment 210699 [details] Patch
Comment on attachment 210699 [details] Patch Clearing flags on attachment: 210699 Committed r155207: <http://trac.webkit.org/changeset/155207>
All reviewed patches have been landed. Closing bug.