The return value of an OnBeforeUnloadEventHandler should always be coerced into a...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 29 Sep 2018 02:03:51 +0000 (02:03 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 29 Sep 2018 02:03:51 +0000 (02:03 +0000)
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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@236633 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/loader/form-submission-after-beforeunload-cancel.html
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSEventListener.cpp

index 7ad4d76..1d4eaf6 100644 (file)
@@ -1,3 +1,17 @@
+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:
+
 2018-09-28  Simon Fraser  <simon.fraser@apple.com>
 
         RenderLayer::removeOnlyThisLayer() should not call updateLayerPositions()
index 196c65b..b87a28b 100644 (file)
@@ -13,7 +13,7 @@ function submitForm()
     document.forms[0].submit();
 }
 
-window.addEventListener("beforeunload", function() {
+onbeforeunload = function() {
 
     if (window._confirmationDialogDisplayedOnce)
         return "Click 'Leave Page'";
@@ -34,7 +34,7 @@ window.addEventListener("beforeunload", function() {
     window._confirmationDialogDisplayedOnce = true;
     
     return "Click 'Stay on Page'";
-});
+}
 </script>
 
 <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>
index 01d0937..46d4973 100644 (file)
@@ -1,5 +1,16 @@
 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.
+
+        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>
+
         document.open() should throw errors for cross-origin calls
         https://bugs.webkit.org/show_bug.cgi?id=189371
         <rdar://problem/44282700>
index 2ab8cc7..0e1f210 100644 (file)
@@ -1,7 +1,7 @@
 
 PASS Returning a string must not cancel the event: CustomEvent, non-cancelable 
 PASS Returning a string must not cancel the event: CustomEvent, cancelable 
-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
+PASS Returning false must not cancel the event, because it's coerced to the DOMString "false" which does not cancel CustomEvents: CustomEvent, cancelable 
 PASS Returning a string must not cancel the event: BeforeUnloadEvent with type "click", cancelable 
 PASS Returning null with a real iframe unloading 
 PASS Returning undefined with a real iframe unloading 
index 3c7f803..3de6497 100644 (file)
@@ -1,3 +1,24 @@
+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):
+
 2018-09-28  Simon Fraser  <simon.fraser@apple.com>
 
         RenderLayer::removeOnlyThisLayer() should not call updateLayerPositions()
index 9931fe8..0613436 100644 (file)
@@ -149,52 +149,63 @@ void JSEventListener::handleEvent(ScriptExecutionContext& scriptExecutionContext
         callType = getCallData(vm, handleEventFunction, callData);
     }
 
-    if (callType != CallType::None) {
-        Ref<JSEventListener> protectedThis(*this);
+    if (callType == CallType::None)
+        return;
 
-        MarkedArgumentBuffer args;
-        args.append(toJS(exec, globalObject, &event));
-        ASSERT(!args.hasOverflowed());
+    Ref<JSEventListener> protectedThis(*this);
 
-        Event* savedEvent = globalObject->currentEvent();
+    MarkedArgumentBuffer args;
+    args.append(toJS(exec, globalObject, &event));
+    ASSERT(!args.hasOverflowed());
 
-        // 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);
+    Event* savedEvent = globalObject->currentEvent();
 
-        VMEntryScope entryScope(vm, vm.entryScope ? vm.entryScope->globalObject() : globalObject);
+    // 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);
 
-        InspectorInstrumentationCookie cookie = JSExecState::instrumentFunctionCall(&scriptExecutionContext, callType, callData);
+    VMEntryScope entryScope(vm, vm.entryScope ? vm.entryScope->globalObject() : globalObject);
 
-        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);
+    InspectorInstrumentationCookie cookie = JSExecState::instrumentFunctionCall(&scriptExecutionContext, callType, callData);
 
-        InspectorInstrumentation::didCallFunction(cookie, &scriptExecutionContext);
+    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);
 
-        globalObject->setCurrentEvent(savedEvent);
+    InspectorInstrumentation::didCallFunction(cookie, &scriptExecutionContext);
 
-        if (is<WorkerGlobalScope>(scriptExecutionContext)) {
-            auto& scriptController = *downcast<WorkerGlobalScope>(scriptExecutionContext).script();
-            bool terminatorCausedException = (scope.exception() && isTerminatedExecutionException(vm, scope.exception()));
-            if (terminatorCausedException || scriptController.isTerminatingExecution())
-                scriptController.forbidExecution();
-        }
+    globalObject->setCurrentEvent(savedEvent);
 
-        if (exception) {
-            event.target()->uncaughtExceptionInEventHandler();
-            reportException(exec, exception);
-        } else {
-            if (is<BeforeUnloadEvent>(event))
-                handleBeforeUnloadEventReturnValue(downcast<BeforeUnloadEvent>(event), convert<IDLNullable<IDLDOMString>>(*exec, retval));
-
-            if (m_isAttribute) {
-                if (retval.isFalse())
-                    event.preventDefault();
-            }
-        }
+    if (is<WorkerGlobalScope>(scriptExecutionContext)) {
+        auto& scriptController = *downcast<WorkerGlobalScope>(scriptExecutionContext).script();
+        bool terminatorCausedException = (scope.exception() && isTerminatedExecutionException(vm, scope.exception()));
+        if (terminatorCausedException || scriptController.isTerminatingExecution())
+            scriptController.forbidExecution();
+    }
+
+    if (exception) {
+        event.target()->uncaughtExceptionInEventHandler();
+        reportException(exec, exception);
+        return;
     }
+
+    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();
 }
 
 bool JSEventListener::operator==(const EventListener& listener) const