onbeforeunload event return value coercion is not per-spec
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 20 Feb 2017 06:36:04 +0000 (06:36 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 20 Feb 2017 06:36:04 +0000 (06:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=168382

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Import test coverage from web-platform-tests. We were failing half the checks
before this patch.

* resources/resource-files.json:
* web-platform-tests/html/browsers/browsing-the-web/unloading-documents/MANIFEST:
* web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-1.html: Added.
* web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt: Added.
* web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html: Added.
* web-platform-tests/html/browsers/browsing-the-web/unloading-documents/w3c-import.log:

Source/WebCore:

Update handling of value returned by onbeforeunload event listeners
to match Firefox and the specification:
- https://html.spec.whatwg.org/#the-event-handler-processing-algorithm (step 4)

Namely, the following changes were made:
- Only set the event's returnValue attribute to the returned value if the attribute
  value is the empty string (so as to not override the attribute value if it has
  explicitly been set by JS).
- Cancel the event when the return value is not null by calling preventDefault().

Additionally, the following changes were made:
- Ask the user to confirm the navigation if the event was canceled, not just if the
  returnValue attribute was set to a non-empty string.
as per:
- https://html.spec.whatwg.org/#prompt-to-unload-a-document (step 8)

Tests: fast/events/before-unload-return-string-conversion.html
       imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html

* bindings/js/JSEventListener.cpp:
(WebCore::handleBeforeUnloadEventReturnValue):
(WebCore::JSEventListener::handleEvent):
* loader/FrameLoader.cpp:
(WebCore::shouldAskForNavigationConfirmation):
(WebCore::FrameLoader::dispatchBeforeUnloadEvent):

LayoutTests:

Add test case to check that the value returned by a beforeunload event handler
is already converted to a string, even if the returnValue attribute is also
set on the BeforeUnloadEvent. The existing code did not handle this properly
and it has been fixed in this patch.

* fast/events/before-unload-return-string-conversion-expected.txt: Added.
* fast/events/before-unload-return-string-conversion.html: Added.
* fast/events/resources/before-unload-return-string-conversion-frame.html: Added.

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/events/before-unload-return-string-conversion-expected.txt [new file with mode: 0644]
LayoutTests/fast/events/before-unload-return-string-conversion.html [new file with mode: 0644]
LayoutTests/fast/events/resources/before-unload-return-string-conversion-frame.html [new file with mode: 0644]
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/resources/resource-files.json
LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/MANIFEST
LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-1.html [new file with mode: 0644]
LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt [new file with mode: 0644]
LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html [new file with mode: 0644]
LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/w3c-import.log
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSEventListener.cpp
Source/WebCore/loader/FrameLoader.cpp

index 0c9d2cd..811a540 100644 (file)
@@ -1,3 +1,19 @@
+2017-02-19  Chris Dumez  <cdumez@apple.com>
+
+        onbeforeunload event return value coercion is not per-spec
+        https://bugs.webkit.org/show_bug.cgi?id=168382
+
+        Reviewed by Darin Adler.
+
+        Add test case to check that the value returned by a beforeunload event handler
+        is already converted to a string, even if the returnValue attribute is also
+        set on the BeforeUnloadEvent. The existing code did not handle this properly
+        and it has been fixed in this patch.
+
+        * fast/events/before-unload-return-string-conversion-expected.txt: Added.
+        * fast/events/before-unload-return-string-conversion.html: Added.
+        * fast/events/resources/before-unload-return-string-conversion-frame.html: Added.
+
 2017-02-18  Ryosuke Niwa  <rniwa@webkit.org>
 
         REGRESSION(r212218): Assertion failures in and after parserRemoveChild
diff --git a/LayoutTests/fast/events/before-unload-return-string-conversion-expected.txt b/LayoutTests/fast/events/before-unload-return-string-conversion-expected.txt
new file mode 100644 (file)
index 0000000..24a14e7
--- /dev/null
@@ -0,0 +1,13 @@
+CONFIRM NAVIGATION: PASS
+Tests that the value returned from a beforeunload event handler gets converted to a string, even if the returnValue property was already set.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS event.defaultPrevented is true
+PASS event.returnValue is "PASS"
+PASS toStringCalled is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/events/before-unload-return-string-conversion.html b/LayoutTests/fast/events/before-unload-return-string-conversion.html
new file mode 100644 (file)
index 0000000..770d6aa
--- /dev/null
@@ -0,0 +1,19 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src="../../resources/js-test-pre.js"></script>
+<script>
+description("Tests that the value returned from a beforeunload event handler gets converted to a string, even if the returnValue property was already set.");
+jsTestIsAsync = true;
+
+const iframe = document.createElement("iframe");
+iframe.onload = function() {
+    iframe.onload = null;
+    iframe.contentWindow.runTest();
+};
+
+iframe.src = "resources/before-unload-return-string-conversion-frame.html";
+document.body.appendChild(iframe);
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
diff --git a/LayoutTests/fast/events/resources/before-unload-return-string-conversion-frame.html b/LayoutTests/fast/events/resources/before-unload-return-string-conversion-frame.html
new file mode 100644 (file)
index 0000000..7387fbe
--- /dev/null
@@ -0,0 +1,25 @@
+<!DOCTYPE html>
+<script>
+
+parent.toStringCalled = false;
+
+window.runTest = function() {
+    window.onbeforeunload = function(e) {
+        e.returnValue = "PASS";
+        return { toString: function () { parent.toStringCalled = true; return "FAIL"; } };
+    }
+
+    const listener = function(e) {
+        parent.event = e;
+        parent.shouldBeTrue("event.defaultPrevented");
+        parent.shouldBeEqualToString("event.returnValue", "PASS");
+        parent.shouldBeTrue("toStringCalled");
+        parent.setTimeout(function() {
+            parent.finishJSTest();
+        }, 0);
+    }
+
+    window.addEventListener("beforeunload", listener);
+    window.location.href = "about:blank";
+}
+</script>
index 8ea7434..e6fb57e 100644 (file)
@@ -1,3 +1,20 @@
+2017-02-19  Chris Dumez  <cdumez@apple.com>
+
+        onbeforeunload event return value coercion is not per-spec
+        https://bugs.webkit.org/show_bug.cgi?id=168382
+
+        Reviewed by Darin Adler.
+
+        Import test coverage from web-platform-tests. We were failing half the checks
+        before this patch.
+
+        * resources/resource-files.json:
+        * web-platform-tests/html/browsers/browsing-the-web/unloading-documents/MANIFEST:
+        * web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-1.html: Added.
+        * web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt: Added.
+        * web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html: Added.
+        * web-platform-tests/html/browsers/browsing-the-web/unloading-documents/w3c-import.log:
+
 2017-02-17  Javier Fernandez  <jfernandez@igalia.com>
 
         [GTK] Unreviewed test gardening
index 4dba1b0..85efdfb 100644 (file)
@@ -66,6 +66,7 @@
         "web-platform-tests/html/browsers/browsing-the-web/navigating-across-documents/navigation_unload_data_url-1.html",
         "web-platform-tests/html/browsers/browsing-the-web/navigating-across-documents/navigation_unload_same_origin-1.html",
         "web-platform-tests/html/browsers/browsing-the-web/unloading-documents/base.html",
+        "web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-1.html",
         "web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-on-history-back-1.html",
         "web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-on-navigation-of-parent-1.html",
         "web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-on-navigation-of-parent-2.html",
         "web-platform-tests/test_keys_wdspec.html",
         "web-platform-tests/upgrade-insecure-requests/support/post-origin-to-parent.html"
     ]
-}
\ No newline at end of file
+}
index f778067..6726618 100644 (file)
@@ -19,6 +19,7 @@ support 005a.html
 support 005b.html
 005.html
 base.html
+support beforeunload-canceling-1.html
 support beforeunload-on-history-back-1.html
 beforeunload-on-history-back.html
 support beforeunload-on-navigation-of-parent-1.html
diff --git a/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-1.html b/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-1.html
new file mode 100644 (file)
index 0000000..26ffa0e
--- /dev/null
@@ -0,0 +1,31 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Support page for beforeunload-canceling.html</title>
+
+<h1>If this goes away, it navigated</h1>
+
+<script>
+"use strict";
+
+window.runTest = (t, { valueToReturn, expectCancelation, setReturnValue, expectedReturnValue }) => {
+  window.onbeforeunload = t.step_func(e => {
+    if (setReturnValue !== undefined) {
+      e.returnValue = setReturnValue;
+    }
+
+    return valueToReturn;
+  });
+
+  const listener = t.step_func(e => {
+    top.assert_equals(e.defaultPrevented, expectCancelation, "canceled");
+    top.assert_equals(e.returnValue, expectedReturnValue, "returnValue");
+    window.onbeforeunload = null;
+
+    t.done();
+  });
+
+  window.addEventListener("beforeunload", listener);
+
+  window.location.href = "about:blank";
+};
+</script>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-expected.txt
new file mode 100644 (file)
index 0000000..296af1a
--- /dev/null
@@ -0,0 +1,26 @@
+CONFIRM NAVIGATION: 
+CONFIRM NAVIGATION: false
+CONFIRM NAVIGATION: true
+CONFIRM NAVIGATION: 0
+CONFIRM NAVIGATION: foo
+CONFIRM NAVIGATION: foo
+CONFIRM NAVIGATION: foo
+CONFIRM NAVIGATION: foo
+CONFIRM NAVIGATION: foo
+CONFIRM NAVIGATION: foo
+
+PASS Returning a string must not cancel the event: CustomEvent, non-cancelable 
+PASS Returning a string must not cancel the event: CustomEvent, cancelable 
+PASS Returning null with a real iframe unloading 
+PASS Returning undefined with a real iframe unloading 
+PASS Returning  with a real iframe unloading 
+PASS Returning false with a real iframe unloading 
+PASS Returning true with a real iframe unloading 
+PASS Returning 0 with a real iframe unloading 
+PASS Returning null with a real iframe unloading; setting returnValue to foo 
+PASS Returning undefined with a real iframe unloading; setting returnValue to foo 
+PASS Returning  with a real iframe unloading; setting returnValue to foo 
+PASS Returning false with a real iframe unloading; setting returnValue to foo 
+PASS Returning true with a real iframe unloading; setting returnValue to foo 
+PASS Returning 0 with a real iframe unloading; setting returnValue to foo 
diff --git a/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html b/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html
new file mode 100644 (file)
index 0000000..b415ac2
--- /dev/null
@@ -0,0 +1,142 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>beforeunload return value cancelation behavior</title>
+<link rel="help" href="https://html.spec.whatwg.org/multipage/webappapis.html#the-event-handler-processing-algorithm">
+<link rel="author" title="Domenic Denicola" href="mailto:d@domenic.me">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<div id="log"></div>
+
+<script>
+"use strict";
+
+async_test(t => {
+  let onbeforeunloadHappened = false;
+  window.onbeforeunload = t.step_func(() => {
+    onbeforeunloadHappened = true;
+    return "cancel me";
+  });
+
+  const listener = t.step_func(e => {
+    assert_true(onbeforeunloadHappened, "CustomEvent must be able to trigger the event handler");
+    assert_false(e.defaultPrevented, "The event must not have been canceled");
+    window.onbeforeunload = null;
+    t.done();
+  });
+
+  window.addEventListener("beforeunload", listener);
+
+  window.dispatchEvent(new CustomEvent("beforeunload"));
+}, "Returning a string must not cancel the event: CustomEvent, non-cancelable");
+
+async_test(t => {
+  let onbeforeunloadHappened = false;
+  window.onbeforeunload = t.step_func(() => {
+    onbeforeunloadHappened = true;
+    return "cancel me";
+  });
+
+  const listener = t.step_func(e => {
+    assert_true(onbeforeunloadHappened, "CustomEvent must be able to trigger the event handler");
+    assert_false(e.defaultPrevented, "The event must not have been canceled");
+    window.onbeforeunload = null;
+    t.done();
+  });
+
+  window.addEventListener("beforeunload", listener);
+
+  window.dispatchEvent(new CustomEvent("beforeunload", { cancelable: true }));
+}, "Returning a string must not cancel the event: CustomEvent, cancelable");
+
+const testCases = [
+  {
+    valueToReturn: null,
+    expectCancelation: false,
+    expectedReturnValue: ""
+  },
+  {
+    valueToReturn: undefined,
+    expectCancelation: false,
+    expectedReturnValue: ""
+  },
+  {
+    valueToReturn: "",
+    expectCancelation: true,
+    expectedReturnValue: ""
+  },
+  {
+    valueToReturn: false,
+    expectCancelation: true,
+    expectedReturnValue: "false"
+  },
+  {
+    valueToReturn: true,
+    expectCancelation: true,
+    expectedReturnValue: "true"
+  },
+  {
+    valueToReturn: 0,
+    expectCancelation: true,
+    expectedReturnValue: "0"
+  },
+  {
+    valueToReturn: null,
+    expectCancelation: false,
+    setReturnValue: "foo",
+    expectedReturnValue: "foo"
+  },
+  {
+    valueToReturn: undefined,
+    expectCancelation: false,
+    setReturnValue: "foo",
+    expectedReturnValue: "foo"
+  },
+  {
+    valueToReturn: "",
+    expectCancelation: true,
+    setReturnValue: "foo",
+    expectedReturnValue: "foo"
+  },
+  {
+    valueToReturn: false,
+    expectCancelation: true,
+    setReturnValue: "foo",
+    expectedReturnValue: "foo"
+  },
+  {
+    valueToReturn: true,
+    expectCancelation: true,
+    setReturnValue: "foo",
+    expectedReturnValue: "foo"
+  },
+  {
+    valueToReturn: 0,
+    expectCancelation: true,
+    setReturnValue: "foo",
+    expectedReturnValue: "foo"
+  }
+];
+
+var testCaseIndex = 0;
+function runNextTest() {
+  const testCase = testCases[testCaseIndex];
+
+  const labelAboutReturnValue = testCase.setReturnValue === undefined ? "" :
+    `; setting returnValue to ${testCase.setReturnValue}`;
+
+  async_test(t => {
+    const iframe = document.createElement("iframe");
+    iframe.onload = t.step_func(() => {
+      iframe.contentWindow.runTest(t, testCase);
+      if (++testCaseIndex < testCases.length)
+        runNextTest();
+    });
+
+    iframe.src = "beforeunload-canceling-1.html";
+    document.body.appendChild(iframe);
+  }, `Returning ${testCase.valueToReturn} with a real iframe unloading${labelAboutReturnValue}`);
+}
+
+runNextTest();
+</script>
index 758d47a..49c19b9 100644 (file)
@@ -21,6 +21,8 @@ List of files:
 /LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/004.html
 /LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/005.html
 /LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/base.html
+/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling-1.html
+/LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html
 /LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-on-history-back-1.html
 /LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-on-history-back.html
 /LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-on-navigation-of-parent-1.html
index 5d49ec9..ae0604d 100644 (file)
@@ -1,3 +1,36 @@
+2017-02-19  Chris Dumez  <cdumez@apple.com>
+
+        onbeforeunload event return value coercion is not per-spec
+        https://bugs.webkit.org/show_bug.cgi?id=168382
+
+        Reviewed by Darin Adler.
+
+        Update handling of value returned by onbeforeunload event listeners
+        to match Firefox and the specification:
+        - https://html.spec.whatwg.org/#the-event-handler-processing-algorithm (step 4)
+
+        Namely, the following changes were made:
+        - Only set the event's returnValue attribute to the returned value if the attribute
+          value is the empty string (so as to not override the attribute value if it has
+          explicitly been set by JS).
+        - Cancel the event when the return value is not null by calling preventDefault().
+
+        Additionally, the following changes were made:
+        - Ask the user to confirm the navigation if the event was canceled, not just if the
+          returnValue attribute was set to a non-empty string.
+        as per:
+        - https://html.spec.whatwg.org/#prompt-to-unload-a-document (step 8)
+
+        Tests: fast/events/before-unload-return-string-conversion.html
+               imported/w3c/web-platform-tests/html/browsers/browsing-the-web/unloading-documents/beforeunload-canceling.html
+
+        * bindings/js/JSEventListener.cpp:
+        (WebCore::handleBeforeUnloadEventReturnValue):
+        (WebCore::JSEventListener::handleEvent):
+        * loader/FrameLoader.cpp:
+        (WebCore::shouldAskForNavigationConfirmation):
+        (WebCore::FrameLoader::dispatchBeforeUnloadEvent):
+
 2017-02-19  Carlos Garcia Campos  <cgarcia@igalia.com>
 
         [SOUP] Call SoupNetworkSession::setShouldIgnoreTLSErrors when testRunner.setAllowsAnySSLCertificate() is called
index 206bd2e..6d51bc7 100644 (file)
@@ -25,6 +25,7 @@
 #include "Event.h"
 #include "Frame.h"
 #include "HTMLElement.h"
+#include "JSDOMConvert.h"
 #include "JSDocument.h"
 #include "JSEvent.h"
 #include "JSEventTarget.h"
@@ -73,6 +74,16 @@ void JSEventListener::visitJSFunction(SlotVisitor& visitor)
     visitor.append(m_jsFunction);
 }
 
+static void handleBeforeUnloadEventReturnValue(BeforeUnloadEvent& event, const String& returnValue)
+{
+    if (returnValue.isNull())
+        return;
+
+    event.preventDefault();
+    if (event.returnValue().isEmpty())
+        event.setReturnValue(returnValue);
+}
+
 void JSEventListener::handleEvent(ScriptExecutionContext* scriptExecutionContext, Event* event)
 {
     ASSERT(scriptExecutionContext);
@@ -159,8 +170,9 @@ void JSEventListener::handleEvent(ScriptExecutionContext* scriptExecutionContext
             event->target()->uncaughtExceptionInEventHandler();
             reportException(exec, exception);
         } else {
-            if (!retval.isUndefinedOrNull() && is<BeforeUnloadEvent>(*event))
-                downcast<BeforeUnloadEvent>(*event).setReturnValue(retval.toWTFString(exec));
+            if (is<BeforeUnloadEvent>(*event))
+                handleBeforeUnloadEventReturnValue(downcast<BeforeUnloadEvent>(*event), convert<IDLNullable<IDLDOMString>>(*exec, retval, StringConversionConfiguration::Normal));
+
             if (m_isAttribute) {
                 if (retval.isFalse())
                     event->preventDefault();
index 68bb5be..9b138bb 100644 (file)
@@ -2975,6 +2975,16 @@ void FrameLoader::dispatchUnloadEvents(UnloadEventPolicy unloadEventPolicy)
         m_frame.document()->removeAllEventListeners();
 }
 
+static bool shouldAskForNavigationConfirmation(const BeforeUnloadEvent& event)
+{
+    // Web pages can request we ask for confirmation before navigating by:
+    // - Cancelling the BeforeUnloadEvent (modern way)
+    // - Setting the returnValue attribute on the BeforeUnloadEvent to a non-empty string.
+    // - Returning a non-empty string from the event handler, which is then set as returnValue
+    //   attribute on the BeforeUnloadEvent.
+    return event.defaultPrevented() || !event.returnValue().isEmpty();
+}
+
 bool FrameLoader::dispatchBeforeUnloadEvent(Chrome& chrome, FrameLoader* frameLoaderBeingNavigated)
 {
     DOMWindow* domWindow = m_frame.document()->domWindow();
@@ -2998,7 +3008,8 @@ bool FrameLoader::dispatchBeforeUnloadEvent(Chrome& chrome, FrameLoader* frameLo
 
     if (!beforeUnloadEvent->defaultPrevented())
         document->defaultEventHandler(beforeUnloadEvent.get());
-    if (beforeUnloadEvent->returnValue().isNull())
+
+    if (!shouldAskForNavigationConfirmation(beforeUnloadEvent))
         return true;
 
     // If the navigating FrameLoader has already shown a beforeunload confirmation panel for the current navigation attempt,