Pages should not be able to abuse users inside beforeunload handlers. Abusive techniques include showing various forms of modal dialogs inside beforeunload and using iframes. Some other browsers don't allow modal dialogs inside beforeunload (like alert, confirm, prompt, showModalDialog), and I think WebKit shouldn't by default. Also, if multiple iframes all try to display a beforeunload confirmation dialog, that seems like spam - Only one should be allowed to be shown. Finally, iframes from different origins probably shouldn't be allowed to show even one beforeunload confirmation. This is in radar as <rdar://problem/14475779>
Created attachment 207037 [details] Patch v1
Comment on attachment 207037 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=207037&action=review > Source/WebCore/ChangeLog:3 > + Need a short description (OOPS!). Yes. > Source/WebCore/loader/FrameLoader.cpp:2760 > -bool FrameLoader::fireBeforeUnloadEvent(Chrome& chrome) > +bool FrameLoader::shouldCloseFiringBeforeUnloadEvent(Chrome& chrome, FrameLoader* navigatingFrameLoader) The new name is difficult to parse, maybe handleBeforeUnloadEvent would be accurate and descriptive enough? > Source/WebCore/loader/FrameLoader.cpp:2777 > if (!beforeUnloadEvent->defaultPrevented()) Below this, there is a document->defaultEventHandler() call. Can it do anything that we'd want to bracket with setFrameHandlingBeforeUnloadDismissal/clearFrameHandlingBeforeUnloadDismissal? > Source/WebCore/loader/FrameLoader.cpp:2798 > + m_frame->page()->console()->addMessage(JSMessageSource, ErrorMessageLevel, "Blocked attempt to show beforeunload confirmation dialog on behalf of a frame with different security origin from the navigating frame. Protocols, domains, and ports must match."); Two spaces? > Source/WebCore/page/DOMWindow.cpp:1037 > + // Pages are not allowed to cause modal alerts during BeforeUnload dispatch. Not sure if I like the capitalization of "BeforeUnload", it's not how the event named. > Source/WebCore/page/DOMWindow.cpp:2017 > + printErrorMessage("Use of window.showModalDialog is not allowed during beforeunload event dispatch."); Add window.print()? :-) > Source/WebCore/page/Page.cpp:1582 > +void Page::setFrameHandlingBeforeUnloadDismissal(Frame* frame) > +{ > + ASSERT(!m_framesHandlingBeforeUnloadDismissal.contains(frame)); > + m_framesHandlingBeforeUnloadDismissal.add(frame); > +} > + > +void Page::clearFrameHandlingBeforeUnloadDismissal(Frame* frame) > +{ > + ASSERT(m_framesHandlingBeforeUnloadDismissal.contains(frame)); > + m_framesHandlingBeforeUnloadDismissal.remove(frame); > +} Can this be a simple number? "setFrameHandlingBeforeUnloadDismissal" doesn't parse very well.
Comment on attachment 207037 [details] Patch v1 Attachment 207037 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1130538 New failing tests: fast/events/before-unload-remove-itself.html fast/loader/page-dismissal-modal-dialogs.html fast/events/onbeforeunload-focused-iframe.html fast/events/onunload-clears-onbeforeunload.html fast/events/before-unload-adopt-within-subframes.html fast/dom/null-page-show-modal-dialog-crash.html http/tests/loading/iframe-beforeunload-dialog-matching-ancestor-securityorigin.html
Created attachment 207044 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.3
Comment on attachment 207037 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=207037&action=review > LayoutTests/ChangeLog:4 > + Need a short description (OOPS!). > + <rdar://problem/14475779> and https://bugs.webkit.org/show_bug.cgi?id=118871. Please update the bug title.
(In reply to comment #3) > (From update of attachment 207037 [details]) > Attachment 207037 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/1130538 > > New failing tests: > fast/events/before-unload-remove-itself.html > fast/loader/page-dismissal-modal-dialogs.html > fast/events/onbeforeunload-focused-iframe.html > fast/events/onunload-clears-onbeforeunload.html > fast/events/before-unload-adopt-within-subframes.html > fast/dom/null-page-show-modal-dialog-crash.html > http/tests/loading/iframe-beforeunload-dialog-matching-ancestor-securityorigin.html These should probably be investigated.
Comment on attachment 207037 [details] Patch v1 Attachment 207037 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1110734 New failing tests: fast/dom/null-page-show-modal-dialog-crash.html fast/events/before-unload-with-subframes.html fast/events/before-unload-adopt-within-subframes.html fast/events/onbeforeunload-focused-iframe.html fast/events/onunload-clears-onbeforeunload.html
Created attachment 207053 [details] Archive of layout-test-results from webkit-ews-06 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.3
I was in the middle of running the entire suite when I uploaded the patch, as I wanted to get the review ball rolling. The failures will almost certainly be due to the fact that alert() can no longer be used for poor-man's logging inside beforeunload, or other reliance on the behavior that is changed. I will, of course, investigate the test failures before pursuing landing this.
(In reply to comment #7) > (From update of attachment 207037 [details]) > Attachment 207037 [details] did not pass mac-ews (mac): > Output: http://webkit-queues.appspot.com/results/1110734 > > New failing tests: > fast/dom/null-page-show-modal-dialog-crash.html > fast/events/before-unload-with-subframes.html > fast/events/before-unload-adopt-within-subframes.html > fast/events/onbeforeunload-focused-iframe.html > fast/events/onunload-clears-onbeforeunload.html These links don't actually go to a results page where you can see the diff between expected and actual =/
Comment on attachment 207037 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=207037&action=review I like this patch, and I have one drive-by comment. >> Source/WebCore/loader/FrameLoader.cpp:2798 >> + m_frame->page()->console()->addMessage(JSMessageSource, ErrorMessageLevel, "Blocked attempt to show beforeunload confirmation dialog on behalf of a frame with different security origin from the navigating frame. Protocols, domains, and ports must match."); > > Two spaces? Also, you can avoid including 'PageConsole.h' if you add these messages to the console via 'm_frame->document()->addConsoleMessage(...)'. See the 'X-Frame-Options' console messages in this file for an example.
Created attachment 207149 [details] Patch v2 - Tests pass in WK1 and WK2 locally, I think...
Mac EWS seems to have gotten hung up. I'll look at landing tomorrow and manually watch for fallout (as mentioned, no failures locally, either WK1 or WK2)
Comment on attachment 207149 [details] Patch v2 - Tests pass in WK1 and WK2 locally, I think... View in context: https://bugs.webkit.org/attachment.cgi?id=207149&action=review > Source/WebCore/loader/FrameLoader.cpp:2759 > +bool FrameLoader::handleBeforeUnloadEvent(Chrome& chrome, FrameLoader* navigatingFrameLoader) Looking at this again, I want to suggest the name "frameLoaderBeingNavigated". What do you think? > Source/WebCore/loader/FrameLoader.cpp:2769 > + > This extra blank line appears accidental. > Source/WebCore/loader/FrameLoader.cpp:2775 > + // We store the frame's page in a local variable because the frame might get detached inside dispatchEvent. > + Page* page = m_frame->page(); > + ASSERT(page); Should it also be protected with a RefPtr? I don't think that this ASSERT is useful - it doesn't help catch a null pointer, we'll get a very clear explanation of what happened when crashing on a dereference.
(In reply to comment #14) > (From update of attachment 207149 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207149&action=review > > > Source/WebCore/loader/FrameLoader.cpp:2759 > > +bool FrameLoader::handleBeforeUnloadEvent(Chrome& chrome, FrameLoader* navigatingFrameLoader) > > Looking at this again, I want to suggest the name "frameLoaderBeingNavigated". What do you think? I'm okay with this. > > Source/WebCore/loader/FrameLoader.cpp:2769 > > + > > > > This extra blank line appears accidental. Indeed! > > Source/WebCore/loader/FrameLoader.cpp:2775 > > + // We store the frame's page in a local variable because the frame might get detached inside dispatchEvent. > > + Page* page = m_frame->page(); > > + ASSERT(page); > > Should it also be protected with a RefPtr? Pages are not ref counted. I think the only action that can possibly involved destruction of a Page would require a cycle of the runloop. > I don't think that this ASSERT is useful - it doesn't help catch a null pointer, we'll get a very clear explanation of what happened when crashing on a dereference. You're right.
wk2 passed, and I think wk1 should be fine... gonna land this then go to bed. http://24.media.tumblr.com/tumblr_lzys30jtSU1rqvy12o1_500.jpg
http://trac.webkit.org/changeset/152941
Comment on attachment 207149 [details] Patch v2 - Tests pass in WK1 and WK2 locally, I think... View in context: https://bugs.webkit.org/attachment.cgi?id=207149&action=review >>> Source/WebCore/loader/FrameLoader.cpp:2775 >>> + ASSERT(page); >> >> Should it also be protected with a RefPtr? >> >> I don't think that this ASSERT is useful - it doesn't help catch a null pointer, we'll get a very clear explanation of what happened when crashing on a dereference. > > Pages are not ref counted. I think the only action that can possibly involved destruction of a Page would require a cycle of the runloop. I am concerned about verifying this hypothesis. We are using the page pointer after dispatching to arbitrary JavaScript. Is it guaranteed that the page object will still be around?