Code inside showModalDialog runs with an incorrect "first window", which can lead to security vulnerability. I have a patch for this, but a regression test is trickier to create.
<rdar://problem/21449949> Could use advice both on creating a regression test and on possible more-elegant ways to fix this.
Created attachment 274574 [details] Patch
Created attachment 274575 [details] Patch
Comment on attachment 274575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274575&action=review No insight into writing a test yet. > Source/WebCore/ChangeLog:3 > + showModalDialog code runs with âfirst windowâ set to wrong window Fancy quotes.
Comment on attachment 274575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274575&action=review I think this change looks good, though the comment about the JSC context makes me wonder if we need to tighten the lifetime of the nullptr JSC scope. Otherwise, I think this change is good. I've asked John W. to help us create a test. Perhaps we could land that test in a separate commit? > Source/WebCore/page/Chrome.cpp:227 > + Your comment here makes it seem like we want the JSC context for 'fireTimersInNestedEventLoop' to be different than the script code for this modal dialog. However, as written we get a new scope for the timers, and this same scope is used for 'runModal'. It's quite likely that you did mean for it to work this way, but if so I think the comment might be misleading. If not, we should control the scope of the new JSC context so that we "pop" back to the original JSC when invoking runModal.
Comment on attachment 274575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=274575&action=review >> Source/WebCore/ChangeLog:3 >> + showModalDialog code runs with âfirst windowâ set to wrong window > > Fancy quotes. Fancy quotes are bad in our review tool, but are they bad in ChangeLog? >> Source/WebCore/page/Chrome.cpp:227 >> + > > Your comment here makes it seem like we want the JSC context for 'fireTimersInNestedEventLoop' to be different than the script code for this modal dialog. However, as written we get a new scope for the timers, and this same scope is used for 'runModal'. It's quite likely that you did mean for it to work this way, but if so I think the comment might be misleading. > > If not, we should control the scope of the new JSC context so that we "pop" back to the original JSC when invoking runModal. JavaScript context has no effect on fireTimersInNestedEventLoop and it doesn’t matter if we do this work before or after calling fireTimersInNestedEventLoop. The name fireTimersInNestedEventLoop might sound like is a function that could execute some JavaScript, but it could not. Calling that function makes sure that run loop timers fire, inside the run loop, instead of waiting for an exit of the outer run loop. All three of the setup steps—PageGroupLoadDeferrer, entryScope, fireTimersInNestedEventLoop—could be done in any order. There are no dependencies between them.
Committed r198575: <http://trac.webkit.org/changeset/198575>
Thank you for fixing this, Darin!