<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[236633] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/236633">236633</a></dd>
<dt>Author</dt> <dd>cdumez@apple.com</dd>
<dt>Date</dt> <dd>2018-09-28 19:03:51 -0700 (Fri, 28 Sep 2018)</dd>
</dl>

<h3>Log Message</h3>
<pre>The return value of an OnBeforeUnloadEventHandler should always be coerced into a DOMString
https://bugs.webkit.org/show_bug.cgi?id=190090

Reviewed by Ryosuke Niwa.

LayoutTests/imported/w3c:

Rebaseline WPT test now that one more check is passing.

* web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt:

Source/WebCore:

The return value of an OnBeforeUnloadEventHandler should always be coerced into a DOMString:
- https://html.spec.whatwg.org/#onbeforeunloadeventhandler
- https://html.spec.whatwg.org/#the-event-handler-processing-algorithm (Step 5)

In particular, this means that returning false in an OnBeforeUnloadEventHandler should NOT
cancel the event when the event is a CustomEvent (and not a BeforeUnloadEvent). This is
because the return value cannot be false at:
- https://html.spec.whatwg.org/#the-event-handler-processing-algorithm (Step 5. Otherwise case).

No new tests, rebaselined existing test.

* bindings/js/JSEventListener.cpp:
(WebCore::JSEventListener::handleEvent):

LayoutTests:

Update test that was returning a value in a beforeunload event listener instead of using an
event handler. The test needs to use an event handler (window.onbeforeunload) as an event
listener does not have a return value. I have verified that our behavior is consistent with
Chrome and Firefox on this test, both with an event listener and an event handler.

* fast/loader/form-submission-after-beforeunload-cancel.html:</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkLayoutTestsfastloaderformsubmissionafterbeforeunloadcancelhtml">trunk/LayoutTests/fast/loader/form-submission-after-beforeunload-cancel.html</a></li>
<li><a href="#trunkLayoutTestsimportedw3cChangeLog">trunk/LayoutTests/imported/w3c/ChangeLog</a></li>
<li><a href="#trunkLayoutTestsimportedw3cwebplatformtestshtmlbrowsersbrowsingthewebunloadingdocumentsbeforeunloadcancelingexpectedtxt">trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCorebindingsjsJSEventListenercpp">trunk/Source/WebCore/bindings/js/JSEventListener.cpp</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (236632 => 236633)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog      2018-09-29 01:53:34 UTC (rev 236632)
+++ trunk/LayoutTests/ChangeLog 2018-09-29 02:03:51 UTC (rev 236633)
</span><span class="lines">@@ -1,3 +1,17 @@
</span><ins>+2018-09-28  Chris Dumez  <cdumez@apple.com>
+
+        The return value of an OnBeforeUnloadEventHandler should always be coerced into a DOMString
+        https://bugs.webkit.org/show_bug.cgi?id=190090
+
+        Reviewed by Ryosuke Niwa.
+
+        Update test that was returning a value in a beforeunload event listener instead of using an
+        event handler. The test needs to use an event handler (window.onbeforeunload) as an event
+        listener does not have a return value. I have verified that our behavior is consistent with
+        Chrome and Firefox on this test, both with an event listener and an event handler.
+
+        * fast/loader/form-submission-after-beforeunload-cancel.html:
+
</ins><span class="cx"> 2018-09-28  Simon Fraser  <simon.fraser@apple.com>
</span><span class="cx"> 
</span><span class="cx">         RenderLayer::removeOnlyThisLayer() should not call updateLayerPositions()
</span></span></pre></div>
<a id="trunkLayoutTestsfastloaderformsubmissionafterbeforeunloadcancelhtml"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/fast/loader/form-submission-after-beforeunload-cancel.html (236632 => 236633)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/fast/loader/form-submission-after-beforeunload-cancel.html     2018-09-29 01:53:34 UTC (rev 236632)
+++ trunk/LayoutTests/fast/loader/form-submission-after-beforeunload-cancel.html        2018-09-29 02:03:51 UTC (rev 236633)
</span><span class="lines">@@ -13,7 +13,7 @@
</span><span class="cx">     document.forms[0].submit();
</span><span class="cx"> }
</span><span class="cx"> 
</span><del>-window.addEventListener("beforeunload", function() {
</del><ins>+onbeforeunload = function() {
</ins><span class="cx"> 
</span><span class="cx">     if (window._confirmationDialogDisplayedOnce)
</span><span class="cx">         return "Click 'Leave Page'";
</span><span class="lines">@@ -34,7 +34,7 @@
</span><span class="cx">     window._confirmationDialogDisplayedOnce = true;
</span><span class="cx">     
</span><span class="cx">     return "Click 'Stay on Page'";
</span><del>-});
</del><ins>+}
</ins><span class="cx"> </script>
</span><span class="cx"> 
</span><span class="cx"> <p>This tests that submitting a form a second time after canceling the first submission in a onbeforeunload handler is allowed. To test manually, follow the instructions in the JavaScript confirmation dialogs.</p>
</span></span></pre></div>
<a id="trunkLayoutTestsimportedw3cChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/imported/w3c/ChangeLog (236632 => 236633)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/imported/w3c/ChangeLog 2018-09-29 01:53:34 UTC (rev 236632)
+++ trunk/LayoutTests/imported/w3c/ChangeLog    2018-09-29 02:03:51 UTC (rev 236633)
</span><span class="lines">@@ -1,5 +1,16 @@
</span><span class="cx"> 2018-09-28  Chris Dumez  <cdumez@apple.com>
</span><span class="cx"> 
</span><ins>+        The return value of an OnBeforeUnloadEventHandler should always be coerced into a DOMString
+        https://bugs.webkit.org/show_bug.cgi?id=190090
+
+        Reviewed by Ryosuke Niwa.
+
+        Rebaseline WPT test now that one more check is passing.
+
+        * web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt:
+
+2018-09-28  Chris Dumez  <cdumez@apple.com>
+
</ins><span class="cx">         document.open() should throw errors for cross-origin calls
</span><span class="cx">         https://bugs.webkit.org/show_bug.cgi?id=189371
</span><span class="cx">         <rdar://problem/44282700>
</span></span></pre></div>
<a id="trunkLayoutTestsimportedw3cwebplatformtestshtmlbrowsersbrowsingthewebunloadingdocumentsbeforeunloadcancelingexpectedtxt"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt (236632 => 236633)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt 2018-09-29 01:53:34 UTC (rev 236632)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt    2018-09-29 02:03:51 UTC (rev 236633)
</span><span class="lines">@@ -1,7 +1,7 @@
</span><span class="cx"> 
</span><span class="cx"> PASS Returning a string must not cancel the event: CustomEvent, non-cancelable 
</span><span class="cx"> PASS Returning a string must not cancel the event: CustomEvent, cancelable 
</span><del>-FAIL Returning false must not cancel the event, because it's coerced to the DOMString "false" which does not cancel CustomEvents: CustomEvent, cancelable assert_false: The event must not have been canceled expected false got true
</del><ins>+PASS Returning false must not cancel the event, because it's coerced to the DOMString "false" which does not cancel CustomEvents: CustomEvent, cancelable 
</ins><span class="cx"> PASS Returning a string must not cancel the event: BeforeUnloadEvent with type "click", cancelable 
</span><span class="cx"> PASS Returning null with a real iframe unloading 
</span><span class="cx"> PASS Returning undefined with a real iframe unloading 
</span></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (236632 => 236633)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog   2018-09-29 01:53:34 UTC (rev 236632)
+++ trunk/Source/WebCore/ChangeLog      2018-09-29 02:03:51 UTC (rev 236633)
</span><span class="lines">@@ -1,3 +1,24 @@
</span><ins>+2018-09-28  Chris Dumez  <cdumez@apple.com>
+
+        The return value of an OnBeforeUnloadEventHandler should always be coerced into a DOMString
+        https://bugs.webkit.org/show_bug.cgi?id=190090
+
+        Reviewed by Ryosuke Niwa.
+
+        The return value of an OnBeforeUnloadEventHandler should always be coerced into a DOMString:
+        - https://html.spec.whatwg.org/#onbeforeunloadeventhandler
+        - https://html.spec.whatwg.org/#the-event-handler-processing-algorithm (Step 5)
+
+        In particular, this means that returning false in an OnBeforeUnloadEventHandler should NOT
+        cancel the event when the event is a CustomEvent (and not a BeforeUnloadEvent). This is
+        because the return value cannot be false at:
+        - https://html.spec.whatwg.org/#the-event-handler-processing-algorithm (Step 5. Otherwise case).
+
+        No new tests, rebaselined existing test.
+
+        * bindings/js/JSEventListener.cpp:
+        (WebCore::JSEventListener::handleEvent):
+
</ins><span class="cx"> 2018-09-28  Simon Fraser  <simon.fraser@apple.com>
</span><span class="cx"> 
</span><span class="cx">         RenderLayer::removeOnlyThisLayer() should not call updateLayerPositions()
</span></span></pre></div>
<a id="trunkSourceWebCorebindingsjsJSEventListenercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/bindings/js/JSEventListener.cpp (236632 => 236633)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/bindings/js/JSEventListener.cpp     2018-09-29 01:53:34 UTC (rev 236632)
+++ trunk/Source/WebCore/bindings/js/JSEventListener.cpp        2018-09-29 02:03:51 UTC (rev 236633)
</span><span class="lines">@@ -149,52 +149,63 @@
</span><span class="cx">         callType = getCallData(vm, handleEventFunction, callData);
</span><span class="cx">     }
</span><span class="cx"> 
</span><del>-    if (callType != CallType::None) {
-        Ref<JSEventListener> protectedThis(*this);
</del><ins>+    if (callType == CallType::None)
+        return;
</ins><span class="cx"> 
</span><del>-        MarkedArgumentBuffer args;
-        args.append(toJS(exec, globalObject, &event));
-        ASSERT(!args.hasOverflowed());
</del><ins>+    Ref<JSEventListener> protectedThis(*this);
</ins><span class="cx"> 
</span><del>-        Event* savedEvent = globalObject->currentEvent();
</del><ins>+    MarkedArgumentBuffer args;
+    args.append(toJS(exec, globalObject, &event));
+    ASSERT(!args.hasOverflowed());
</ins><span class="cx"> 
</span><del>-        // window.event should not be set when the target is inside a shadow tree, as per the DOM specification.
-        bool isTargetInsideShadowTree = is<Node>(event.currentTarget()) && downcast<Node>(*event.currentTarget()).isInShadowTree();
-        if (!isTargetInsideShadowTree)
-            globalObject->setCurrentEvent(&event);
</del><ins>+    Event* savedEvent = globalObject->currentEvent();
</ins><span class="cx"> 
</span><del>-        VMEntryScope entryScope(vm, vm.entryScope ? vm.entryScope->globalObject() : globalObject);
</del><ins>+    // window.event should not be set when the target is inside a shadow tree, as per the DOM specification.
+    bool isTargetInsideShadowTree = is<Node>(event.currentTarget()) && downcast<Node>(*event.currentTarget()).isInShadowTree();
+    if (!isTargetInsideShadowTree)
+        globalObject->setCurrentEvent(&event);
</ins><span class="cx"> 
</span><del>-        InspectorInstrumentationCookie cookie = JSExecState::instrumentFunctionCall(&scriptExecutionContext, callType, callData);
</del><ins>+    VMEntryScope entryScope(vm, vm.entryScope ? vm.entryScope->globalObject() : globalObject);
</ins><span class="cx"> 
</span><del>-        JSValue thisValue = handleEventFunction == jsFunction ? toJS(exec, globalObject, event.currentTarget()) : jsFunction;
-        NakedPtr<JSC::Exception> exception;
-        JSValue retval = JSExecState::profiledCall(exec, JSC::ProfilingReason::Other, handleEventFunction, callType, callData, thisValue, args, exception);
</del><ins>+    InspectorInstrumentationCookie cookie = JSExecState::instrumentFunctionCall(&scriptExecutionContext, callType, callData);
</ins><span class="cx"> 
</span><del>-        InspectorInstrumentation::didCallFunction(cookie, &scriptExecutionContext);
</del><ins>+    JSValue thisValue = handleEventFunction == jsFunction ? toJS(exec, globalObject, event.currentTarget()) : jsFunction;
+    NakedPtr<JSC::Exception> exception;
+    JSValue retval = JSExecState::profiledCall(exec, JSC::ProfilingReason::Other, handleEventFunction, callType, callData, thisValue, args, exception);
</ins><span class="cx"> 
</span><del>-        globalObject->setCurrentEvent(savedEvent);
</del><ins>+    InspectorInstrumentation::didCallFunction(cookie, &scriptExecutionContext);
</ins><span class="cx"> 
</span><del>-        if (is<WorkerGlobalScope>(scriptExecutionContext)) {
-            auto& scriptController = *downcast<WorkerGlobalScope>(scriptExecutionContext).script();
-            bool terminatorCausedException = (scope.exception() && isTerminatedExecutionException(vm, scope.exception()));
-            if (terminatorCausedException || scriptController.isTerminatingExecution())
-                scriptController.forbidExecution();
-        }
</del><ins>+    globalObject->setCurrentEvent(savedEvent);
</ins><span class="cx"> 
</span><del>-        if (exception) {
-            event.target()->uncaughtExceptionInEventHandler();
-            reportException(exec, exception);
-        } else {
-            if (is<BeforeUnloadEvent>(event))
-                handleBeforeUnloadEventReturnValue(downcast<BeforeUnloadEvent>(event), convert<IDLNullable<IDLDOMString>>(*exec, retval));
</del><ins>+    if (is<WorkerGlobalScope>(scriptExecutionContext)) {
+        auto& scriptController = *downcast<WorkerGlobalScope>(scriptExecutionContext).script();
+        bool terminatorCausedException = (scope.exception() && isTerminatedExecutionException(vm, scope.exception()));
+        if (terminatorCausedException || scriptController.isTerminatingExecution())
+            scriptController.forbidExecution();
+    }
</ins><span class="cx"> 
</span><del>-            if (m_isAttribute) {
-                if (retval.isFalse())
-                    event.preventDefault();
-            }
-        }
</del><ins>+    if (exception) {
+        event.target()->uncaughtExceptionInEventHandler();
+        reportException(exec, exception);
+        return;
</ins><span class="cx">     }
</span><ins>+
+    if (!m_isAttribute) {
+        // This is an EventListener and there is therefore no need for any return value handling.
+        return;
+    }
+
+    // Do return value handling for event handlers (https://html.spec.whatwg.org/#the-event-handler-processing-algorithm).
+
+    if (event.type() == eventNames().beforeunloadEvent) {
+        // This is a OnBeforeUnloadEventHandler, and therefore the return value must be coerced into a String.
+        if (is<BeforeUnloadEvent>(event))
+            handleBeforeUnloadEventReturnValue(downcast<BeforeUnloadEvent>(event), convert<IDLNullable<IDLDOMString>>(*exec, retval));
+        return;
+    }
+
+    if (retval.isFalse())
+        event.preventDefault();
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> bool JSEventListener::operator==(const EventListener& listener) const
</span></span></pre>
</div>
</div>

</body>
</html>