Changeset 93567 in webkit

Timestamp:

Aug 22, 2011 5:43:32 PM (13 years ago)

Author:

ap@apple.com

Message:

showModalDialog does not correctly return the defined returnValue in case domain relaxing is used
​https://bugs.webkit.org/show_bug.cgi?id=53191
<rdar://problem/8629478>

Reviewed by Geoff Garen.

Cannot test domain relaxing, we only have 127.0.0.1 and localhost.

• bindings/js/JSDOMWindowCustom.cpp: (WebCore::JSDOMWindow::getOwnPropertySlot): Added a FIXME about a difference with Firefox.

(WebCore::DialogHandler::DialogHandler):
(WebCore::DialogHandler::dialogCreated):
(WebCore::DialogHandler::returnValue):
Changed to fetch returnValue from the global object that's in the frame when the dialog is
dismissed. A dialog can navigate itself, and it also creates a new JSDOMWindow on first load
if the origin doesn't match opener origin (which the case with domain relaxing).
Re-added a security check for returnValue that got lost in r73829, so that we don't send the
result across origins. This matches Firefox.

• bindings/js/JSDOMWindowShell.cpp: (WebCore::JSDOMWindowShell::setWindow): Added an assertion against a very confusing case that should no longer occur.

• page/Frame.cpp: (WebCore::Frame::pageDestroyed): Don't clear the window shell, it doesn't seem necessary, but prevents DialogHandler from fetching the return value. Added a keepAlive call to avoid changing life support timer behavior in this patch.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• bindings/js/JSDOMWindowCustom.cpp (modified) (3 diffs)
• bindings/js/JSDOMWindowShell.cpp (modified) (1 diff)
• page/Frame.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-08-22  Alexey Proskuryakov  <ap@apple.com>
+
+        showModalDialog does not correctly return the defined returnValue in case domain relaxing is used
+        https://bugs.webkit.org/show_bug.cgi?id=53191
+        <rdar://problem/8629478>
+
+        Reviewed by Geoff Garen.
+
+        Cannot test domain relaxing, we only have 127.0.0.1 and localhost.
+
+        * bindings/js/JSDOMWindowCustom.cpp: (WebCore::JSDOMWindow::getOwnPropertySlot):
+        Added a FIXME about a difference with Firefox.
+
+        (WebCore::DialogHandler::DialogHandler):
+        (WebCore::DialogHandler::dialogCreated):
+        (WebCore::DialogHandler::returnValue):
+        Changed to fetch returnValue from the global object that's in the frame when the dialog is
+        dismissed. A dialog can navigate itself, and it also creates a new JSDOMWindow on first load
+        if the origin doesn't match opener origin (which the case with domain relaxing).
+        Re-added a security check for returnValue that got lost in r73829, so that we don't send the
+        result across origins. This matches Firefox.
+
+        * bindings/js/JSDOMWindowShell.cpp: (WebCore::JSDOMWindowShell::setWindow): Added an assertion
+        against a very confusing case that should no longer occur.
+
+        * page/Frame.cpp: (WebCore::Frame::pageDestroyed): Don't clear the window shell, it doesn't
+        seem necessary, but prevents DialogHandler from fetching the return value. Added a keepAlive
+        call to avoid changing life support timer behavior in this patch.
+
 2011-08-22  Alice Boxhall  <aboxhall@chromium.org>
 

trunk/Source/WebCore/bindings/js/JSDOMWindowCustom.cpp

@@ -128 +128 @@
     const HashEntry* entry;
 
-    // We don't want any properties other than "close" and "closed" on a closed window.
+    // 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 doesn't fully match Firefox, which allows at least toString in addition to those.
     if (!impl()->frame()) {
         // The following code is safe for cross-domain and same domain use.
@@ -645 +647 @@
     explicit DialogHandler(ExecState* exec)
         : m_exec(exec)
-        , m_globalObject(0)
     {
     }
@@ -654 +655 @@
 private:
     ExecState* m_exec;
-    JSDOMWindow* m_globalObject;
+    RefPtr<Frame> m_frame;
 };
 
 inline void DialogHandler::dialogCreated(DOMWindow* dialog)
 {
+    m_frame = dialog->frame();
     // FIXME: This looks like a leak between the normal world and an isolated
     //        world if dialogArguments comes from an isolated world.
-    m_globalObject = toJSDOMWindow(dialog->frame(), normalWorld(m_exec->globalData()));
+    JSDOMWindow* globalObject = toJSDOMWindow(m_frame.get(), normalWorld(m_exec->globalData()));
     if (JSValue dialogArguments = m_exec->argument(1))
-        m_globalObject->putDirect(m_exec->globalData(), Identifier(m_exec, "dialogArguments"), dialogArguments);
+        globalObject->putDirect(m_exec->globalData(), Identifier(m_exec, "dialogArguments"), dialogArguments);
 }
 
 inline JSValue DialogHandler::returnValue() const
 {
-    if (!m_globalObject)
+    JSDOMWindow* globalObject = toJSDOMWindow(m_frame.get(), normalWorld(m_exec->globalData()));
+    if (!globalObject)
         return jsUndefined();
     Identifier identifier(m_exec, "returnValue");
     PropertySlot slot;
-    if (!m_globalObject->JSGlobalObject::getOwnPropertySlot(m_exec, identifier, slot))
+    if (!globalObject->getOwnPropertySlot(m_exec, identifier, slot))
         return jsUndefined();
     return slot.getValue(m_exec, identifier);

trunk/Source/WebCore/bindings/js/JSDOMWindowShell.cpp

@@ -58 +58 @@
 void JSDOMWindowShell::setWindow(PassRefPtr<DOMWindow> domWindow)
 {
+    // Replacing JSDOMWindow via telling JSDOMWindowShell to use the same DOMWindow it already uses makes no sense,
+    // so we'd better never try to.
+    ASSERT(!m_window || domWindow.get() != m_window->impl());
     // Explicitly protect the global object's prototype so it isn't collected
     // when we allocate the global object. (Once the global object is fully

trunk/Source/WebCore/page/Frame.cpp

@@ -732 +732 @@
 void Frame::pageDestroyed()
 {
+    // FIXME: Rename this function, since it's called not only from Page destructor, but in several other cases.
+    // This cleanup is needed whenever we remove a frame from page.
+
     if (Frame* parent = tree()->parent())
         parent->loader()->checkLoadComplete();
@@ -750 +753 @@
         page()->focusController()->setFocusedFrame(0);
 
-    script()->clearWindowShell();
+    // FIXME: Removing keepalive will make behavior better match Firefox.
+    // Some code that used to be here indirectly triggered keepalive, and we don't want to change behavior at the moment.
+    keepAlive();
+
     script()->clearScriptObjects();
     script()->updatePlatformScriptObjects();