RESOLVED FIXED 53191
showModalDialog does not correctly return the defined returnValue in case domain relaxing is used 
https://bugs.webkit.org/show_bug.cgi?id=53191
Summary showModalDialog does not correctly return the defined returnValue in case dom...
Brent
Reported 2011-01-26 13:50:52 PST
Created attachment 80230 [details] Sample web pages/scripts to reproduce showModalDialog does not correctly return the defined returnValue in case domain relaxing is used, even if the opener window and the modal dialog content are properly set to the same domain. Ver: 534.17+ In some web applications content from different systems are embedded inside of iframes and need to communicate with each other, so it is common to have domain relaxing enabled, to allow JavaScript communication across the iframes. We discovered that showModalDialog does not correctly return the defined returnValue in case domain relaxing is used, even if the opener window and the modal dialog content are properly set to the same domain. Prerequisites to reproduce the issue: 1. Copy the modaldialog-folder containing the three files index.html, dialogbroken.html and dialogworkaround.html to a local webserver 2. Add the following line to your /etc/hosts 127.0.0.1 localhost.test.domain 3.Disable the proxy server in the browser (if any) Steps to reproduce: 1. Open WebKit and load the page from your server using http://localhost/modaldialog/ 2. Click on "Open modal dialog" 3. Click on "Close and return TEST" 4. The alert-window shows the proper return value "TEST" 5. Now change the URL to http://localhost.test.domain/modaldialog/ 6. Click on "Open modal dialog" again 7. Click on "Close and return TEST" The alert-window now returns "undefined" instead of "TEST" As a workaround it is possible to store the return value in the parent window, which can be accessed using window.opener, and to adapt the code which reads the return value accordingly.
Attachments
Sample web pages/scripts to reproduce (2.48 KB, application/zip)
2011-01-26 13:50 PST, Brent 		no flags
Details
proposed fix (4.72 KB, patch)
2011-08-22 17:02 PDT, Alexey Proskuryakov 		no flags
DetailsFormatted DiffDiff
proposed fix (6.64 KB, patch)
2011-08-22 17:12 PDT, Alexey Proskuryakov 		ggaren: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1 2011-02-23 17:34:09 PST
<rdar://problem/8629478>
Alexey Proskuryakov
Comment 2 2011-08-17 17:05:09 PDT
Moreover, dialogArguments variable isn't available in modal dialog in this case.
Alexey Proskuryakov
Comment 3 2011-08-22 16:33:11 PDT
> Moreover, dialogArguments variable isn't available in modal dialog in this case. OK, that doesn't work in Firefox or IE either. Makes sense, since the engine doesn't yet know if the dialog is going to use domain relaxing, and passing data cross origin is not allowed.
Alexey Proskuryakov
Comment 4 2011-08-22 17:02:03 PDT
Created attachment 104766 [details] proposed fix
Alexey Proskuryakov
Comment 5 2011-08-22 17:12:08 PDT
Created attachment 104769 [details] proposed fix Changed to actually successfully print an error message.
Geoffrey Garen
Comment 6 2011-08-22 17:32:34 PDT
Comment on attachment 104769 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=104769&action=review r=me with those changes > Source/WebCore/ChangeLog:19 > + dismissed. A dialog can navigate itself, and it also creates a new JSDOMWindow on firt load s/firt/first/ > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:678 > + JSDOMWindow* globalObject = toJSDOMWindow(m_frame.get(), normalWorld(m_exec->globalData())); > + if (!globalObject) > + return jsUndefined(); > + if (!asJSDOMWindow(m_exec->lexicalGlobalObject())->allowsAccessFrom(globalObject->globalExec())) > return jsUndefined(); > Identifier identifier(m_exec, "returnValue"); > PropertySlot slot; > - if (!m_globalObject->JSGlobalObject::getOwnPropertySlot(m_exec, identifier, slot)) > + if (!globalObject->JSGlobalObject::getOwnPropertySlot(m_exec, identifier, slot)) There's no need to do this allowsAccessFrom check ourselves; it's built into the window object. Just call the normal getOwnPropertySlot, instead of skipping window checks by calling JSGlobalObject::getOwnPropertySlot directly. By the way, make sure to call out this additional security check in your ChangeLog, and explain why you added it (to match Firefox).
Alexey Proskuryakov
Comment 7 2011-08-22 17:44:08 PDT
Committed <http://trac.webkit.org/changeset/93567>
Xan Lopez
Comment 8 2011-08-24 14:42:37 PDT
This broke a GTK+ API test. Our DOM objects from the GObject bindings are not being collected anymore, since we don't get a frame destruction notification when doing: create view, get refs to DOM objects, destroy view. I guess this is because of the keepAlive() call added to pageDestroyed... not sure if the bug is ours or not, but in any case this has changed behavior without adding new tests.
Alexey Proskuryakov
Comment 9 2011-08-24 14:51:54 PDT
The keepAlive call was always there, just called indirectly. It's quite possible that there is a real bug introduced here, and one that affects platforms other than Gtk, but since the only known change is on Gtk, it's not practical for me to investigate what's going on.
Xan Lopez
Comment 10 2011-08-24 15:37:36 PDT
(In reply to comment #9) > The keepAlive call was always there, just called indirectly. keepAlive() is only called for the JS bindings. Our unit tests do not have any JS, so that's likely the reason they broke (since they never had the keepAlive() call in the first place). > > It's quite possible that there is a real bug introduced here, and one that affects platforms other than Gtk, but since the only known change is on Gtk, it's not practical for me to investigate what's going on. I guess this might or might not affect other !JS bindings, but not sure.
Note You need to log in before you can comment on or make changes to this bug.