We should not return undefined for most properties of a detached Window
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Jan 2018 03:31:28 +0000 (03:31 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Jan 2018 03:31:28 +0000 (03:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181416
<rdar://problem/36162489>

Reviewed by Ryosuke Niwa.

LayoutTests/imported/w3c:

Rebaseline several WPT tests now that more checks are passing.

* web-platform-tests/custom-elements/custom-element-registry/per-global-expected.txt:
* web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe-append-to-child-document-expected.txt:
* web-platform-tests/service-workers/service-worker/detached-context.https-expected.txt:

Source/WebCore:

We should not return undefined for most properties on a detached Window. WebKit previously only exposed "closed"
and "close" properties on detached / frameless windows. However, this does not match the HTML specification [1]
or the behavior of Firefox and Chrome.

Note that Chrome does not seem to fully follow the HTML specification either, it seems to treat detached windows
the same way as cross-origin ones. As a result, it only exposed properties that are visible cross-origin when
a window is detached / frameless.

[1] https://html.spec.whatwg.org/#windowproxy-get

No new tests, updated existingt tests.

* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::jsDOMWindowGetOwnPropertySlotRestrictedAccess):
(WebCore::JSDOMWindow::getOwnPropertySlot):
(WebCore::JSDOMWindow::getOwnPropertySlotByIndex):

LayoutTests:

Update existing tests to reflect behavior change.

* fast/dom/Window/orphaned-frame-access-expected.txt:
* fast/dom/Window/orphaned-frame-access.html:
* fast/frames/detached-frame-property-expected.txt:
* fast/frames/detached-frame-property.html:

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

LayoutTests/ChangeLog
LayoutTests/fast/dom/Window/orphaned-frame-access-expected.txt
LayoutTests/fast/dom/Window/orphaned-frame-access.html
LayoutTests/fast/frames/detached-frame-property-expected.txt
LayoutTests/fast/frames/detached-frame-property.html
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/custom-elements/custom-element-registry/per-global-expected.txt
LayoutTests/imported/w3c/web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe-append-to-child-document-expected.txt
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/detached-context.https-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSDOMWindowCustom.cpp

index ce8f9ab..091e3f9 100644 (file)
@@ -1,3 +1,18 @@
+2018-01-09  Chris Dumez  <cdumez@apple.com>
+
+        We should not return undefined for most properties of a detached Window
+        https://bugs.webkit.org/show_bug.cgi?id=181416
+        <rdar://problem/36162489>
+
+        Reviewed by Ryosuke Niwa.
+
+        Update existing tests to reflect behavior change.
+
+        * fast/dom/Window/orphaned-frame-access-expected.txt:
+        * fast/dom/Window/orphaned-frame-access.html:
+        * fast/frames/detached-frame-property-expected.txt:
+        * fast/frames/detached-frame-property.html:
+
 2018-01-09  Darin Adler  <darin@apple.com>
 
         Further refinement to list item and counter code after "list-item" counter fix
index daedf30..88b36ea 100644 (file)
@@ -1,3 +1,3 @@
 This tests access to a window with a null frame. You should see "PASS" for each of the three tests below.
 
-property: PASS ... array: PASS ... missing property: PASS.
+property: PASS ... array: PASS ... constructor: PASS .... operation: PASS.
index a61e683..6b8a19e 100644 (file)
@@ -1,7 +1,6 @@
-<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Strict//EN">
+<!DOCTYPE html>
 <html>
 <head>
-<meta HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=UTF-8">
 <title>Null frame access tests</title>
 <script>
 window.onload = function() {
@@ -17,9 +16,10 @@ window.onload = function() {
 
     // schedule to run after the frame is null
     setTimeout(function() {
-        document.body.appendChild(document.createTextNode(win.test || 'property: PASS ... '));
-        document.body.appendChild(document.createTextNode(win[20] || 'array: PASS ... '));
-        document.body.appendChild(document.createTextNode(win.Comment || 'missing property: PASS.'));
+        document.body.appendChild(document.createTextNode(win.test || 'property: FAIL ... '));
+        document.body.appendChild(document.createTextNode(win[20] || 'array: FAIL ... '));
+        document.body.appendChild(document.createTextNode(win.Comment ? 'constructor: PASS .... ' : 'constructor: FAIL ... '));
+        document.body.appendChild(document.createTextNode(win.postMessage ? 'operation: PASS.' : 'operation: FAIL.'));
         if (window.testRunner)
             testRunner.notifyDone();
     }, 0);
@@ -28,6 +28,6 @@ window.onload = function() {
 </head>
 <body>
 This tests access to a window with a null frame. You should see "PASS" for each of the three tests below.<br /><br />
-<iframe id="subframe" src="data:text/html,&lt;script&gt;window.test='FAIL ... ';window[20]='FAIL ... ';&lt;/script&gt;" />
+<iframe id="subframe" srcdoc="&lt;script&gt;window.test='property: PASS ... ';window[20]='array: PASS ... ';&lt;/script&gt;" />
 </body>
 </html>
index 50839bd..b725799 100644 (file)
@@ -1,4 +1,30 @@
-Test that properties in a detached subframe become unavailable immediately.
+Checks the value of detached subframe properties.
 
-PASS
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS !!detachedWindow.postMessage is true
+PASS !!detachedWindow.close is true
+PASS detachedWindow.closed is true
+PASS detachedWindow.top is null
+PASS detachedWindow.opener is null
+PASS detachedWindow.parent is null
+PASS detachedWindow.frameElement is null
+PASS detachedWindow.window is null
+PASS detachedWindow.frames is null
+PASS detachedWindow.self is null
+PASS !detachedWindow.navigator is true
+PASS !detachedWindow.locationbar is true
+PASS !detachedWindow.history is true
+PASS !detachedWindow.localStorage is true
+PASS !!detachedWindow.document is true
+PASS !!detachedWindow.XMLHttpRequest is true
+PASS !!detachedWindow.getComputedStyle is true
+PASS !detachedWindow.screen is true
+PASS detachedWindow.innerHeight is 0
+PASS detachedWindow.innerWidth is 0
+PASS !detachedWindow.location is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
 
index 7f69873..2382bee 100644 (file)
@@ -1,14 +1,41 @@
-<p>Test that properties in a detached subframe become unavailable immediately.</p>
+<script src="../../resources/js-test.js"></script>
 <script>
-if (window.testRunner)
-    testRunner.dumpAsText();
-
+description("Checks the value of detached subframe properties.");
 onload = function()
 {
-    var childWindow = frames[0];
+    detachedWindow = frames[0];
     document.body.removeChild(document.getElementsByTagName("iframe")[0]);
-    document.getElementById("result").innerHTML = childWindow.XMLHttpRequest ? "FAIL" : "PASS";
+
+    // Chrome and Firefox agree with us.
+    shouldBeTrue("!!detachedWindow.postMessage");
+    shouldBeTrue("!!detachedWindow.close");
+    shouldBeTrue("detachedWindow.closed");
+    shouldBeNull("detachedWindow.top");
+    shouldBeNull("detachedWindow.opener");
+    shouldBeNull("detachedWindow.parent");
+    shouldBeNull("detachedWindow.frameElement"); // Technically, Chrome returns undefined here, not null.
+
+    // Chrome agrees with us but Firefox returns the detachedWindow.
+    shouldBeNull("detachedWindow.window");
+    shouldBeNull("detachedWindow.frames");
+    shouldBeNull("detachedWindow.self");
+
+    // Chrome returns undefined but Firefox has a valid object.
+    shouldBeTrue("!detachedWindow.navigator");
+    shouldBeTrue("!detachedWindow.locationbar");
+    shouldBeTrue("!detachedWindow.history");
+    shouldBeTrue("!detachedWindow.localStorage");
+    shouldBeTrue("!!detachedWindow.document");
+    shouldBeTrue("!!detachedWindow.XMLHttpRequest");
+    shouldBeTrue("!!detachedWindow.getComputedStyle");
+
+    // Chrome returns undefined but Firefox throws an exception.
+    shouldBeTrue("!detachedWindow.screen");
+    shouldBe("detachedWindow.innerHeight", "0");
+    shouldBe("detachedWindow.innerWidth", "0");
+
+    // Both Chrome and Firefox disagree with us and return a valid object.
+    shouldBeTrue("!detachedWindow.location");
 }
 </script>
-<div id="result">FAIL: Script did not run</div>
 <iframe src="about:blank"></iframe>
index 2822525..88f0f72 100644 (file)
@@ -1,3 +1,17 @@
+2018-01-09  Chris Dumez  <cdumez@apple.com>
+
+        We should not return undefined for most properties of a detached Window
+        https://bugs.webkit.org/show_bug.cgi?id=181416
+        <rdar://problem/36162489>
+
+        Reviewed by Ryosuke Niwa.
+
+        Rebaseline several WPT tests now that more checks are passing.
+
+        * web-platform-tests/custom-elements/custom-element-registry/per-global-expected.txt:
+        * web-platform-tests/html/semantics/embedded-content/the-iframe-element/iframe-append-to-child-document-expected.txt:
+        * web-platform-tests/service-workers/service-worker/detached-context.https-expected.txt:
+
 2018-01-09  Matt Lewis  <jlewis3@apple.com>
 
         Unreviewed, rolling out r226531.
index f397a3c..50eeb9c 100644 (file)
@@ -2,7 +2,7 @@
 
 Harness Error (TIMEOUT), message = null
 
-FAIL Discarding the browsing context must not change window.customElements assert_equals: expected (object) object "[object CustomElementRegistry]" but got (undefined) undefined
+PASS Discarding the browsing context must not change window.customElements 
 FAIL Navigating from the initial about:blank must not replace window.customElements assert_equals: expected object "[object CustomElementRegistry]" but got object "[object CustomElementRegistry]"
 FAIL document.open() must replace window.customElements assert_not_equals: got disallowed value object "[object CustomElementRegistry]"
 
index 7fc10bc..bde430c 100644 (file)
@@ -4,5 +4,5 @@ PASS accessing a ServiceWorkerRegistration from a removed iframe
 PASS accessing a ServiceWorker object from a removed iframe 
 PASS accessing navigator.serviceWorker on a detached iframe 
 FAIL accessing navigator on a removed frame assert_throws: function "() => get_navigator()" did not throw
-FAIL accessing navigator.serviceWorker on a removed about:blank frame undefined is not an object (evaluating 'get_navigator().serviceWorker')
+FAIL accessing navigator.serviceWorker on a removed about:blank frame null is not an object (evaluating 'get_navigator().serviceWorker')
 
index 473e072..2a525ba 100644 (file)
@@ -1,3 +1,28 @@
+2018-01-09  Chris Dumez  <cdumez@apple.com>
+
+        We should not return undefined for most properties of a detached Window
+        https://bugs.webkit.org/show_bug.cgi?id=181416
+        <rdar://problem/36162489>
+
+        Reviewed by Ryosuke Niwa.
+
+        We should not return undefined for most properties on a detached Window. WebKit previously only exposed "closed"
+        and "close" properties on detached / frameless windows. However, this does not match the HTML specification [1]
+        or the behavior of Firefox and Chrome.
+
+        Note that Chrome does not seem to fully follow the HTML specification either, it seems to treat detached windows
+        the same way as cross-origin ones. As a result, it only exposed properties that are visible cross-origin when
+        a window is detached / frameless.
+
+        [1] https://html.spec.whatwg.org/#windowproxy-get
+
+        No new tests, updated existingt tests.
+
+        * bindings/js/JSDOMWindowCustom.cpp:
+        (WebCore::jsDOMWindowGetOwnPropertySlotRestrictedAccess):
+        (WebCore::JSDOMWindow::getOwnPropertySlot):
+        (WebCore::JSDOMWindow::getOwnPropertySlotByIndex):
+
 2018-01-09  Darin Adler  <darin@apple.com>
 
         Further refinement to list item and counter code after "list-item" counter fix
index 63f0839..9a92d16 100644 (file)
@@ -85,26 +85,6 @@ static bool jsDOMWindowGetOwnPropertySlotRestrictedAccess(JSDOMWindow* thisObjec
 
     auto& builtinNames = static_cast<JSVMClientData*>(vm.clientData)->builtinNames();
 
-    // We don't want any properties other than "close" and "closed" on a frameless window
-    // (i.e. one whose page got closed, or whose iframe got removed).
-    // FIXME: This handling for frameless windows duplicates similar behaviour for cross-origin
-    // access below; we should try to find a way to merge the two.
-    if (!frame) {
-        if (propertyName == builtinNames.closedPublicName()) {
-            slot.setCustom(thisObject, JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum, jsDOMWindowClosed);
-            return true;
-        }
-        if (propertyName == builtinNames.closePublicName()) {
-            slot.setCustom(thisObject, JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionClose, 0>);
-            return true;
-        }
-
-        // FIXME: We should have a message here that explains why the property access/function call was
-        // not allowed. 
-        slot.setUndefined();
-        return true;
-    }
-
     // https://html.spec.whatwg.org/#crossorigingetownpropertyhelper-(-o,-p-)
     if (propertyName == vm.propertyNames->toStringTagSymbol || propertyName == vm.propertyNames->hasInstanceSymbol || propertyName == vm.propertyNames->isConcatSpreadableSymbol) {
         slot.setValue(thisObject, JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::DontEnum, jsUndefined());
@@ -162,9 +142,11 @@ static bool jsDOMWindowGetOwnPropertySlotRestrictedAccess(JSDOMWindow* thisObjec
     // not match IE, but some sites end up naming frames things that conflict with window
     // properties that are in Moz but not IE. Since we have some of these, we have to do it
     // the Moz way.
-    if (auto* scopedChild = frame->tree().scopedChild(propertyNameToAtomicString(propertyName))) {
-        slot.setValue(thisObject, JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum, toJS(exec, scopedChild->document()->domWindow()));
-        return true;
+    if (frame) {
+        if (auto* scopedChild = frame->tree().scopedChild(propertyNameToAtomicString(propertyName))) {
+            slot.setValue(thisObject, JSC::PropertyAttribute::ReadOnly | JSC::PropertyAttribute::DontDelete | JSC::PropertyAttribute::DontEnum, toJS(exec, scopedChild->document()->domWindow()));
+            return true;
+        }
     }
 
     throwSecurityError(*exec, scope, errorMessage);
@@ -186,9 +168,9 @@ bool JSDOMWindow::getOwnPropertySlot(JSObject* object, ExecState* state, Propert
     auto* thisObject = jsCast<JSDOMWindow*>(object);
     auto* frame = thisObject->wrapped().frame();
 
-    // Hand off all cross-domain/frameless access to jsDOMWindowGetOwnPropertySlotRestrictedAccess.
+    // Hand off all cross-domain access to jsDOMWindowGetOwnPropertySlotRestrictedAccess.
     String errorMessage;
-    if (!frame || !BindingSecurity::shouldAllowAccessToDOMWindow(*state, thisObject->wrapped(), errorMessage))
+    if (!BindingSecurity::shouldAllowAccessToDOMWindow(*state, thisObject->wrapped(), errorMessage))
         return jsDOMWindowGetOwnPropertySlotRestrictedAccess(thisObject, frame, state, propertyName, slot, errorMessage);
     
     // FIXME: this need more explanation.
@@ -200,7 +182,7 @@ bool JSDOMWindow::getOwnPropertySlot(JSObject* object, ExecState* state, Propert
     if (Base::getOwnPropertySlot(thisObject, state, propertyName, slot)) {
         // Detect when we're getting the property 'showModalDialog', this is disabled, and has its original value.
         bool isShowModalDialogAndShouldHide = propertyName == static_cast<JSVMClientData*>(state->vm().clientData)->builtinNames().showModalDialogPublicName()
-            && !DOMWindow::canShowModalDialog(*frame)
+            && (!frame || !DOMWindow::canShowModalDialog(*frame))
             && slot.isValue() && isHostFunction(slot.getValue(state, propertyName), jsDOMWindowInstanceFunctionShowModalDialog);
         // Unless we're in the showModalDialog special case, we're done.
         if (!isShowModalDialogAndShouldHide)
@@ -239,7 +221,7 @@ bool JSDOMWindow::getOwnPropertySlotByIndex(JSObject* object, ExecState* state,
 
     // Hand off all cross-domain/frameless access to jsDOMWindowGetOwnPropertySlotRestrictedAccess.
     String errorMessage;
-    if (!frame || !BindingSecurity::shouldAllowAccessToDOMWindow(*state, thisObject->wrapped(), errorMessage))
+    if (!BindingSecurity::shouldAllowAccessToDOMWindow(*state, thisObject->wrapped(), errorMessage))
         return jsDOMWindowGetOwnPropertySlotRestrictedAccess(thisObject, frame, state, Identifier::from(state, index), slot, errorMessage);
 
     // (2) Regular own properties.