|Summary:||showModalDialog does not correctly return the defined returnValue in case domain relaxing is used|
|Severity:||Normal||CC:||abarth, ap, xan.lopez|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.6|
|Bug Depends on:||66749, 66894, 66895|
Description Brent 2011-01-26 13:50:52 PST
Comment 2 Alexey Proskuryakov 2011-08-17 17:05:09 PDT
Moreover, dialogArguments variable isn't available in modal dialog in this case.
Comment 3 Alexey Proskuryakov 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.
Comment 4 Alexey Proskuryakov 2011-08-22 17:02:03 PDT
Created attachment 104766 [details] proposed fix
Comment 5 Alexey Proskuryakov 2011-08-22 17:12:08 PDT
Created attachment 104769 [details] proposed fix Changed to actually successfully print an error message.
Comment 6 Geoffrey Garen 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).
Comment 7 Alexey Proskuryakov 2011-08-22 17:44:08 PDT
Comment 8 Xan Lopez 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.
Comment 9 Alexey Proskuryakov 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.
Comment 10 Xan Lopez 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.