Bug 120156

Summary:

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

Product:

WebKit

Reporter:

Vivek Galatage <vivekg>

Component:

WebCore JavaScript

Assignee:

Vivek Galatage <vivekg>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, buildbot, commit-queue, darin, joepeck, mkwst, rniwa, sam, timothy

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion	none
Patch	none
Patch	none
Patch	none

Vivek Galatage

Reported 2013-08-22 05:55:11 PDT

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.

Attachments
Patch (9.19 KB, patch) 2013-08-22 06:12 PDT, Vivek Galatage	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (289.71 KB, application/zip) 2013-08-22 06:52 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion (331.45 KB, application/zip) 2013-08-22 07:17 PDT, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (306.59 KB, application/zip) 2013-08-22 07:32 PDT, Build Bot	no flags	Details
Patch (1.64 KB, patch) 2013-08-26 04:46 PDT, Vivek Galatage	no flags	Details Formatted Diff Diff
Patch (1.58 KB, patch) 2013-08-27 06:01 PDT, Vivek Galatage	no flags	Details Formatted Diff Diff
Patch (1.60 KB, patch) 2013-09-05 21:18 PDT, Vivek Galatage	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Vivek Galatage

Comment 1 2013-08-22 06:12:41 PDT

Created attachment 209354 [details] Patch

Ryosuke Niwa

Comment 2 2013-08-22 06:31:56 PDT

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.

Build Bot

Comment 3 2013-08-22 06:52:40 PDT

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

Build Bot

Comment 4 2013-08-22 06:52:41 PDT

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

Vivek Galatage

Comment 5 2013-08-22 07:15:01 PDT

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

Build Bot

Comment 6 2013-08-22 07:16:58 PDT

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

Build Bot

Comment 7 2013-08-22 07:17:00 PDT

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

Build Bot

Comment 8 2013-08-22 07:32:38 PDT

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

Build Bot

Comment 9 2013-08-22 07:32:40 PDT

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

Alexey Proskuryakov

Comment 10 2013-08-22 09:18:30 PDT

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.

Vivek Galatage

Comment 11 2013-08-26 04:45:28 PDT

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

Vivek Galatage

Comment 12 2013-08-26 04:46:35 PDT

Created attachment 209639 [details] Patch

Vivek Galatage

Comment 13 2013-08-26 04:48:28 PDT

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

Alexey Proskuryakov

Comment 14 2013-08-26 09:47:50 PDT

If the test opens a child window, will it be able to close itself?

Darin Adler

Comment 15 2013-08-26 10:10:48 PDT

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.

Vivek Galatage

Comment 16 2013-08-26 10:32:54 PDT

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

Vivek Galatage

Comment 17 2013-08-26 10:36:50 PDT

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

Vivek Galatage

Comment 18 2013-08-27 06:01:13 PDT

Created attachment 209758 [details] Patch

Darin Adler

Comment 19 2013-08-27 09:50:12 PDT

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.

Timothy Hatcher

Comment 20 2013-08-27 12:01:27 PDT

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

Darin Adler

Comment 21 2013-08-27 14:46:20 PDT

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

Timothy Hatcher

Comment 22 2013-08-27 15:03:22 PDT

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

Vivek Galatage

Comment 23 2013-09-05 21:18:45 PDT

Created attachment 210699 [details] Patch

WebKit Commit Bot

Comment 24 2013-09-06 12:59:50 PDT

Comment on attachment 210699 [details] Patch Clearing flags on attachment: 210699 Committed r155207: <http://trac.webkit.org/changeset/155207>

WebKit Commit Bot

Comment 25 2013-09-06 12:59:53 PDT

All reviewed patches have been landed. Closing bug.

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