[webkit-reviews] review granted: [Bug 118871] Pages should not be able to abuse users inside beforeunload handlers : [Attachment 207037] Patch v1

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 118871: Pages should not be able to abuse users inside beforeunload
handlers
https://bugs.webkit.org/show_bug.cgi?id=118871

Attachment 207037: Patch v1
https://bugs.webkit.org/attachment.cgi?id=207037&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.