Attachment 275681 Details for Bug 156227 – Patch

[patch] Patch

bug-156227-20160405114703.patch (text/plain), 19.25 KB, created by Chris Dumez on 2016-04-05 11:47:03 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2016-04-05 11:47:03 PDT

Size: 19.25 KB

patch

obsolete

>Subversion Revision: 199013
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index ac7bd8426ae7c2115551539b4e9d17bff563379e..c66ed1feb3c5a4063e9fcd2c733d7086ca2041d2 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,51 @@
>+2016-04-04  Chris Dumez  <cdumez@apple.com>
>+
>+        MessageEvent.source window is incorrect once window has been reified
>+        https://bugs.webkit.org/show_bug.cgi?id=156227
>+        <rdar://problem/25545831>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        MessageEvent.source window was incorrect once window had been reified.
>+
>+        If the Window had not been reified, we kept constructing new
>+        postMessage() functions when calling window.postMessage(). We used to
>+        pass activeDOMWindow(execState) as source Window to
>+        DOMWindow::postMessage(). activeDOMWindow() uses
>+        exec->lexicalGlobalObject() which did the right thing because we
>+        used to construct a new postMessage() function in the caller's context.
>+
>+        However, after reification, due to the way JSDOMWindow::getOwnPropertySlot()
>+        was implemented, we would stop constructing new postMessage() functions
>+        when calling window.postMessage(). As a result, the source window would
>+        become incorrect because exec->lexicalGlobalObject() would return the
>+        target Window instead.
>+
>+        In this patch, the following is done:
>+        1. Stop constructing a new function every time in the same origin case
>+           for postMessage, blur, focus and close. This was inefficient and lead
>+           to incorrect behavior:
>+           - The behavior would differ depending if the Window is reified or not
>+           - It would be impossible to delete those operations, which is
>+             incompatible with the specification and other browsers (tested
>+             Firefox and Chrome).
>+        2. Use callerDOMWindow(execState) instead of activeDOMWindow(execState)
>+           as source Window in JSDOMWindow::handlePostMessage(). callerDOMWindow()
>+           is a new utility function that returns the caller's Window object.
>+
>+        Tests: fast/dom/Window/delete-operations.html
>+               fast/dom/Window/messageevent-source-postmessage-reified.html
>+               fast/dom/Window/messageevent-source-postmessage.html
>+               fast/dom/Window/window-postmessage-clone-frames.html
>+
>+        * bindings/js/JSDOMBinding.cpp:
>+        (WebCore::GetCallerCodeBlockFunctor::operator()):
>+        (WebCore::GetCallerCodeBlockFunctor::codeBlock):
>+        (WebCore::callerDOMWindow):
>+        * bindings/js/JSDOMBinding.h:
>+        * bindings/js/JSDOMWindowCustom.cpp:
>+        (WebCore::handlePostMessage):
>+
> 2016-04-04  Anders Carlsson  <andersca@apple.com>
> 
>         Properly generate static functions that return Promises
>diff --git a/Source/WebCore/bindings/js/JSDOMBinding.cpp b/Source/WebCore/bindings/js/JSDOMBinding.cpp
>index 20b9ff507492c6b942f6ddb409c33ad40323f8a4..d26bc66906db11ebe4b987d48df1c318a5f5ccee 100644
>--- a/Source/WebCore/bindings/js/JSDOMBinding.cpp
>+++ b/Source/WebCore/bindings/js/JSDOMBinding.cpp
>@@ -35,6 +35,7 @@
> #include "JSDOMWindowCustom.h"
> #include "JSExceptionBase.h"
> #include "SecurityOrigin.h"
>+#include <bytecode/CodeBlock.h>
> #include <inspector/ScriptCallStack.h>
> #include <inspector/ScriptCallStackFactory.h>
> #include <interpreter/Interpreter.h>
>@@ -557,6 +558,36 @@ uint64_t toUInt64(ExecState* exec, JSValue value, IntegerConversionConfiguration
>     return n;
> }
> 
>+class GetCallerCodeBlockFunctor {
>+public:
>+    GetCallerCodeBlockFunctor() = default;
>+
>+    StackVisitor::Status operator()(StackVisitor& visitor)
>+    {
>+        if (!m_hasSkippedFirstFrame) {
>+            m_hasSkippedFirstFrame = true;
>+            return StackVisitor::Continue;
>+        }
>+
>+        m_codeBlock = visitor->codeBlock();
>+        return StackVisitor::Done;
>+    }
>+
>+    CodeBlock* codeBlock() const { return m_codeBlock; }
>+
>+private:
>+    bool m_hasSkippedFirstFrame { false };
>+    CodeBlock* m_codeBlock { nullptr };
>+};
>+
>+DOMWindow* callerDOMWindow(ExecState* exec)
>+{
>+    GetCallerCodeBlockFunctor iter;
>+    exec->iterate(iter);
>+    CodeBlock* codeBlock = iter.codeBlock();
>+    return codeBlock ? &asJSDOMWindow(codeBlock->globalObject())->wrapped() : nullptr;
>+}
>+
> DOMWindow& activeDOMWindow(ExecState* exec)
> {
>     return asJSDOMWindow(exec->lexicalGlobalObject())->wrapped();
>diff --git a/Source/WebCore/bindings/js/JSDOMBinding.h b/Source/WebCore/bindings/js/JSDOMBinding.h
>index c8902e69be707f6d869293be1e75b14ef868eacf..7a57bc67d5063d97bee89dd7f4726c4ee8b85051 100644
>--- a/Source/WebCore/bindings/js/JSDOMBinding.h
>+++ b/Source/WebCore/bindings/js/JSDOMBinding.h
>@@ -78,6 +78,7 @@ struct ExceptionDetails {
> 
> typedef int ExceptionCode;
> 
>+DOMWindow* callerDOMWindow(JSC::ExecState*);
> DOMWindow& activeDOMWindow(JSC::ExecState*);
> DOMWindow& firstDOMWindow(JSC::ExecState*);
> 
>diff --git a/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp b/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
>index 22ea1dfdc3105d545b251674767fd9d6b7c39d47..6408a8ee0bb288bebcffcb95d37a9a33fb19bce5 100644
>--- a/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
>+++ b/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
>@@ -252,30 +252,6 @@ bool JSDOMWindow::getOwnPropertySlot(JSObject* object, ExecState* exec, Property
>     // (Particularly, is it correct that this exists here but not in getOwnPropertySlotByIndex?)
>     slot.setWatchpointSet(thisObject->m_windowCloseWatchpoints);
> 
>-    // FIXME: These are all bogus. Keeping these here make some tests pass that check these properties
>-    // are own properties of the window, but introduces other problems instead (e.g. if you overwrite
>-    // & delete then the original value is restored!) Should be removed.
>-    if (propertyName == exec->propertyNames().blur) {
>-        if (!Base::getOwnPropertySlot(thisObject, exec, propertyName, slot))
>-            slot.setCustom(thisObject, ReadOnly | DontDelete | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionBlur, 0>);
>-        return true;
>-    }
>-    if (propertyName == exec->propertyNames().close) {
>-        if (!Base::getOwnPropertySlot(thisObject, exec, propertyName, slot))
>-            slot.setCustom(thisObject, ReadOnly | DontDelete | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionClose, 0>);
>-        return true;
>-    }
>-    if (propertyName == exec->propertyNames().focus) {
>-        if (!Base::getOwnPropertySlot(thisObject, exec, propertyName, slot))
>-            slot.setCustom(thisObject, ReadOnly | DontDelete | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionFocus, 0>);
>-        return true;
>-    }
>-    if (propertyName == exec->propertyNames().postMessage) {
>-        if (!Base::getOwnPropertySlot(thisObject, exec, propertyName, slot))
>-            slot.setCustom(thisObject, ReadOnly | DontDelete | DontEnum, nonCachingStaticFunctionGetter<jsDOMWindowInstanceFunctionPostMessage, 2>);
>-        return true;
>-    }
>-
>     if (propertyName == exec->propertyNames().showModalDialog) {
>         if (Base::getOwnPropertySlot(thisObject, exec, propertyName, slot))
>             return true;
>@@ -612,8 +588,14 @@ static JSValue handlePostMessage(DOMWindow& impl, ExecState& state)
>     if (state.hadException())
>         return jsUndefined();
> 
>+    DOMWindow* callerWindow = callerDOMWindow(&state);
>+    if (!callerWindow) {
>+        setDOMException(&state, TypeError);
>+        return jsUndefined();
>+    }
>+
>     ExceptionCode ec = 0;
>-    impl.postMessage(message.release(), &messagePorts, targetOrigin, activeDOMWindow(&state), ec);
>+    impl.postMessage(message.release(), &messagePorts, targetOrigin, *callerWindow, ec);
>     setDOMException(&state, ec);
> 
>     return jsUndefined();
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 9dc1df0ddf321a44f1ef5042b5c735f527806d6d..f04b46119a024260daa5f6f3b2f8358dec621141 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,27 @@
>+2016-04-04  Chris Dumez  <cdumez@apple.com>
>+
>+        MessageEvent.source window is incorrect once window has been reified
>+        https://bugs.webkit.org/show_bug.cgi?id=156227
>+        <rdar://problem/25545831>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add tests that cover using MessageEvent.source Window for messaging
>+        using postMessage(). There are 2 versions of the test, one where the
>+        main window is reified and one where it is not. The test that has a
>+        reified main window was failing because this fix.
>+
>+        * fast/dom/Window/delete-operations-expected.txt: Added.
>+        * fast/dom/Window/delete-operations.html: Added.
>+        Make sure that operations on Window are indeed deletable. Previously,
>+        it would be impossible to delete postMessage, blur, focus and close.
>+
>+        * fast/dom/Window/messageevent-source-postmessage-expected.txt: Added.
>+        * fast/dom/Window/messageevent-source-postmessage-reified-expected.txt: Added.
>+        * fast/dom/Window/messageevent-source-postmessage-reified.html: Added.
>+        * fast/dom/Window/messageevent-source-postmessage.html: Added.
>+        * fast/dom/Window/resources/messageevent-source-postmessage-frame.html: Added.
>+
> 2016-04-04  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Marking plugins/focus.html as flaky on mac
>diff --git a/LayoutTests/fast/dom/Window/delete-operations-expected.txt b/LayoutTests/fast/dom/Window/delete-operations-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..8d35f089d7e5f7b0930be4386963362b99f7e036
>--- /dev/null
>+++ b/LayoutTests/fast/dom/Window/delete-operations-expected.txt
>@@ -0,0 +1,75 @@
>+Tests deleting window operations works as expected
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS window.postMessage is an instance of Function
>+window.postMessage = 1
>+PASS window.postMessage is 1
>+PASS delete window.postMessage is true
>+PASS window.postMessage is undefined.
>+
>+PASS window.focus is an instance of Function
>+window.focus = 1
>+PASS window.focus is 1
>+PASS delete window.focus is true
>+PASS window.focus is undefined.
>+
>+PASS window.blur is an instance of Function
>+window.blur = 1
>+PASS window.blur is 1
>+PASS delete window.blur is true
>+PASS window.blur is undefined.
>+
>+PASS window.close is an instance of Function
>+window.close = 1
>+PASS window.close is 1
>+PASS delete window.close is true
>+PASS window.close is undefined.
>+
>+PASS window.open is an instance of Function
>+window.open = 1
>+PASS window.open is 1
>+PASS delete window.open is true
>+PASS window.open is undefined.
>+
>+PASS window.showModalDialog is an instance of Function
>+window.showModalDialog = 1
>+PASS window.showModalDialog is 1
>+PASS delete window.showModalDialog is true
>+PASS window.showModalDialog is undefined.
>+
>+PASS window.alert is an instance of Function
>+window.alert = 1
>+PASS window.alert is 1
>+PASS delete window.alert is true
>+PASS window.alert is undefined.
>+
>+PASS window.confirm is an instance of Function
>+window.confirm = 1
>+PASS window.confirm is 1
>+PASS delete window.confirm is true
>+PASS window.confirm is undefined.
>+
>+PASS window.prompt is an instance of Function
>+window.prompt = 1
>+PASS window.prompt is 1
>+PASS delete window.prompt is true
>+PASS window.prompt is undefined.
>+
>+PASS window.stop is an instance of Function
>+window.stop = 1
>+PASS window.stop is 1
>+PASS delete window.stop is true
>+PASS window.stop is undefined.
>+
>+PASS window.scroll is an instance of Function
>+window.scroll = 1
>+PASS window.scroll is 1
>+PASS delete window.scroll is true
>+PASS window.scroll is undefined.
>+
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/fast/dom/Window/delete-operations.html b/LayoutTests/fast/dom/Window/delete-operations.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..3a757e05658fb0eed55384340803b1a320827b41
>--- /dev/null
>+++ b/LayoutTests/fast/dom/Window/delete-operations.html
>@@ -0,0 +1,31 @@
>+<!DOCTYPE html>
>+<body>
>+<script src="../../../resources/js-test-pre.js"></script>
>+<script>
>+description("Tests deleting window operations works as expected");
>+
>+function testFunction(functionName)
>+{
>+    shouldBeType("window." + functionName, "Function");
>+    evalAndLog("window." + functionName + " = 1");
>+    shouldBe("window." + functionName, "1");
>+    shouldBeTrue("delete window." + functionName);
>+    shouldBeUndefined("window." + functionName);
>+    debug("");
>+}
>+
>+testFunction("postMessage");
>+testFunction("focus");
>+testFunction("blur");
>+testFunction("close");
>+testFunction("open");
>+testFunction("showModalDialog");
>+testFunction("alert");
>+testFunction("confirm");
>+testFunction("prompt");
>+testFunction("stop");
>+testFunction("scroll");
>+
>+</script>
>+<script src="../../../resources/js-test-post.js"></script>
>+</body>
>diff --git a/LayoutTests/fast/dom/Window/messageevent-source-postmessage-expected.txt b/LayoutTests/fast/dom/Window/messageevent-source-postmessage-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..a6db670822f159e7e9cd0adbc5f2e74bab13fa49
>--- /dev/null
>+++ b/LayoutTests/fast/dom/Window/messageevent-source-postmessage-expected.txt
>@@ -0,0 +1,35 @@
>+Tests that MessageEvent.source is correct and can be used for messaging.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+* Sending message 1 to child
>+* Parent received message 2 from child
>+PASS messageEvent.source is frames[0]
>+PASS messageEvent.data is counter + 1
>+* Sending message 3 to child
>+* Parent received message 4 from child
>+PASS messageEvent.source is frames[0]
>+PASS messageEvent.data is counter + 1
>+* Sending message 5 to child
>+* Parent received message 6 from child
>+PASS messageEvent.source is frames[0]
>+PASS messageEvent.data is counter + 1
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>+
>+--------
>+Frame: '-->'
>+--------
>+* Child received message 1 from parent
>+PASS messageEvent.source is parent
>+* Sending message 2 to parent
>+* Child received message 3 from parent
>+PASS messageEvent.source is parent
>+* Sending message 4 to parent
>+* Child received message 5 from parent
>+PASS messageEvent.source is parent
>+* Sending message 6 to parent
>+
>diff --git a/LayoutTests/fast/dom/Window/messageevent-source-postmessage-reified-expected.txt b/LayoutTests/fast/dom/Window/messageevent-source-postmessage-reified-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..b7bbcfd9436b1a030f9dcde36d127d93a3c5f46c
>--- /dev/null
>+++ b/LayoutTests/fast/dom/Window/messageevent-source-postmessage-reified-expected.txt
>@@ -0,0 +1,35 @@
>+Tests that MessageEvent.source is correct and can be used for messaging (reified Window case).
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+* Sending message 1 to child
>+* Parent received message 2 from child
>+PASS messageEvent.source is frames[0]
>+PASS messageEvent.data is counter + 1
>+* Sending message 3 to child
>+* Parent received message 4 from child
>+PASS messageEvent.source is frames[0]
>+PASS messageEvent.data is counter + 1
>+* Sending message 5 to child
>+* Parent received message 6 from child
>+PASS messageEvent.source is frames[0]
>+PASS messageEvent.data is counter + 1
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>+
>+--------
>+Frame: '-->'
>+--------
>+* Child received message 1 from parent
>+PASS messageEvent.source is parent
>+* Sending message 2 to parent
>+* Child received message 3 from parent
>+PASS messageEvent.source is parent
>+* Sending message 4 to parent
>+* Child received message 5 from parent
>+PASS messageEvent.source is parent
>+* Sending message 6 to parent
>+
>diff --git a/LayoutTests/fast/dom/Window/messageevent-source-postmessage-reified.html b/LayoutTests/fast/dom/Window/messageevent-source-postmessage-reified.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..a7891134f5024c14756045c93df208ecd7b92741
>--- /dev/null
>+++ b/LayoutTests/fast/dom/Window/messageevent-source-postmessage-reified.html
>@@ -0,0 +1,42 @@
>+<!DOCTYPE html>
>+<html>
>+<script src="../../../resources/js-test-pre.js"></script>
>+<body onload="runTest()">
>+<script>
>+description("Tests that MessageEvent.source is correct and can be used for messaging (reified Window case).");
>+jsTestIsAsync = true;
>+
>+// Reify the window.
>+window.test = 1;
>+delete window.test;
>+
>+if (window.testRunner)
>+    testRunner.dumpChildFramesAsText();
>+
>+counter = 1;
>+
>+window.onmessage = function(e) {
>+    debug("* Parent received message " + e.data + " from child");
>+    messageEvent = e;
>+    shouldBe("messageEvent.source", "frames[0]");
>+    shouldBe("messageEvent.data", "counter + 1");
>+    if (messageEvent.data > 5) {
>+        finishJSTest();
>+        return;
>+    }
>+    counter = messageEvent.data + 1;
>+    debug("* Sending message " + counter + " to child");
>+    messageEvent.source.postMessage(counter, "*");
>+}
>+
>+function runTest()
>+{
>+    debug("* Sending message " + counter + " to child");
>+    frames[0].postMessage(counter, "*");
>+}
>+
>+</script>
>+<iframe src="resources/messageevent-source-postmessage-frame.html"></iframe>
>+<script src="../../../resources/js-test-post.js"></script>
>+</body>
>+</html>
>diff --git a/LayoutTests/fast/dom/Window/messageevent-source-postmessage.html b/LayoutTests/fast/dom/Window/messageevent-source-postmessage.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..9867310f45f08215de96408b49f3f97ab577c76b
>--- /dev/null
>+++ b/LayoutTests/fast/dom/Window/messageevent-source-postmessage.html
>@@ -0,0 +1,38 @@
>+<!DOCTYPE html>
>+<html>
>+<script src="../../../resources/js-test-pre.js"></script>
>+<body onload="runTest()">
>+<script>
>+description("Tests that MessageEvent.source is correct and can be used for messaging.");
>+jsTestIsAsync = true;
>+
>+if (window.testRunner)
>+    testRunner.dumpChildFramesAsText();
>+
>+counter = 1;
>+
>+window.onmessage = function(e) {
>+    debug("* Parent received message " + e.data + " from child");
>+    messageEvent = e;
>+    shouldBe("messageEvent.source", "frames[0]");
>+    shouldBe("messageEvent.data", "counter + 1");
>+    if (messageEvent.data > 5) {
>+        finishJSTest();
>+        return;
>+    }
>+    counter = messageEvent.data + 1;
>+    debug("* Sending message " + counter + " to child");
>+    messageEvent.source.postMessage(counter, "*");
>+}
>+
>+function runTest()
>+{
>+    debug("* Sending message " + counter + " to child");
>+    frames[0].postMessage(counter, "*");
>+}
>+
>+</script>
>+<iframe src="resources/messageevent-source-postmessage-frame.html"></iframe>
>+<script src="../../../resources/js-test-post.js"></script>
>+</body>
>+</html>
>diff --git a/LayoutTests/fast/dom/Window/resources/messageevent-source-postmessage-frame.html b/LayoutTests/fast/dom/Window/resources/messageevent-source-postmessage-frame.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..246dc3df3024235b7150f5cd368c3c9708758ccf
>--- /dev/null
>+++ b/LayoutTests/fast/dom/Window/resources/messageevent-source-postmessage-frame.html
>@@ -0,0 +1,16 @@
>+<!DOCTYPE html>
>+<body>
>+<script src="../../../../resources/js-test-pre.js"></script>
>+<script>
>+jsTestIsAsync = true;
>+
>+window.onmessage = function(e) {
>+    debug("* Child received message " + e.data + " from parent");
>+    messageEvent = e;
>+    shouldBe("messageEvent.source", "parent");
>+    debug("* Sending message " + (e.data + 1) + " to parent");
>+    messageEvent.source.postMessage(e.data + 1, "*");
>+}
>+</script>
>+<script src="../../../../resources/js-test-post.js"></script>
>+</body>

Actions: View | Formatted Diff | Diff

Attachments on bug 156227: 275640 | 275645 | 275681 | 275709 | 275711 | 275722