Bug 150980 – alert, confirm, prompt, showModalDialog should be forbidden during page close and navigation

• Component: New Bugs
• Status: RESOLVED FIXED

[Geoffrey Garen]

alert, confirm, prompt, showModalDialog should be forbidden during page close and navigation

[Geoffrey Garen]

Created attachment 264965 [details]
Patch

[Geoffrey Garen]

Created attachment 264967 [details]
Patch

[Geoffrey Garen]

<rdar://problem/23435543>

[Brady Eidson]

Comment on attachment 264967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264967&action=review

> Source/WebCore/page/DOMWindow.cpp:1055
> +        printErrorMessage("Use of window.print is not allowed during navigation.");

I don't like this wording, as "during navigation" isn't very precise. I think a user - even a web developer - would colloquially consider "while a new page is still loading" to be "during navigation".

Perhaps "Use of window.print is not allowed while unloading a page", or some variant thereof?

> Source/WebCore/page/DOMWindow.cpp:1087
> +        printErrorMessage("Use of window.alert is not allowed during navigation.");

Same comment

> Source/WebCore/page/DOMWindow.cpp:1106
> +        printErrorMessage("Use of window.confirm is not allowed during navigation.");

Same comment.

> Source/WebCore/page/DOMWindow.cpp:1125
> +        printErrorMessage("Use of window.prompt is not allowed during navigation.");

Same.

> Source/WebCore/page/DOMWindow.cpp:2241
> +        printErrorMessage("Use of window.showModalDialog is not allowed during navigation.");

You guessed it.

[Geoffrey Garen]

> Perhaps "Use of window.print is not allowed while unloading a page"

OK.

[Geoffrey Garen]

Created attachment 265118 [details]
Patch

[Build Bot]

Comment on attachment 265118 [details]
Patch

Attachment 265118 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/407704

New failing tests:
fast/events/onunload-not-on-body.html
fast/dom/Geolocation/notimer-after-unload.html
fast/history/timed-refresh-in-cached-frame.html
fast/loader/page-dismissal-modal-dialogs.html
fast/events/unload-confirm.html
fast/events/pagehide-confirm.html
fast/events/unload-prompt.html
fast/events/beforeunload-alert.html
compositing/iframes/page-cache-layer-tree.html
fast/events/pageshow-pagehide-on-back-cached.html
fast/events/pageshow-pagehide-on-back-uncached.html
fast/events/onunload-window-property.html
fast/events/pagehide-alert.html
fast/events/onunload.html
fast/events/beforeunload-confirm.html
fast/events/unload-alert.html
fast/loader/frames-with-unload-handlers-in-page-cache.html
fast/events/beforeunload-prompt.html
fast/events/pagehide-prompt.html

[Build Bot]

Created attachment 265128 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 265118 [details]
Patch

Attachment 265118 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/407743

New failing tests:
fast/events/beforeunload-showModalDialog.html
fast/events/beforeunload-confirm.html
fast/dom/Geolocation/notimer-after-unload.html
fast/history/timed-refresh-in-cached-frame.html
fast/events/unload-prompt.html
compositing/iframes/page-cache-layer-tree.html
fast/events/pageshow-pagehide-on-back-cached.html
fast/events/pageshow-pagehide-on-back-uncached.html
fast/events/onunload.html
fast/events/beforeunload-prompt.html
fast/events/onunload-not-on-body.html
fast/loader/page-dismissal-modal-dialogs.html
fast/events/unload-confirm.html
fast/events/pagehide-confirm.html
fast/events/beforeunload-alert.html
fast/events/pagehide-showModalDialog.html
fast/events/onunload-window-property.html
fast/events/pagehide-prompt.html
fast/events/unload-alert.html
fast/events/pagehide-alert.html
fast/events/unload-showModalDialog.html
fast/loader/frames-with-unload-handlers-in-page-cache.html

[Build Bot]

Created attachment 265129 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 265118 [details]
Patch

Attachment 265118 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/407798

New failing tests:
fast/events/beforeunload-showModalDialog.html
fast/events/beforeunload-confirm.html
fast/dom/Geolocation/notimer-after-unload.html
fast/history/timed-refresh-in-cached-frame.html
fast/events/unload-prompt.html
compositing/iframes/page-cache-layer-tree.html
fast/events/pageshow-pagehide-on-back-cached.html
fast/events/pageshow-pagehide-on-back-uncached.html
fast/events/onunload.html
fast/events/beforeunload-prompt.html
fast/events/onunload-not-on-body.html
fast/loader/page-dismissal-modal-dialogs.html
fast/events/unload-confirm.html
fast/events/pagehide-confirm.html
fast/events/beforeunload-alert.html
fast/events/pagehide-showModalDialog.html
fast/events/onunload-window-property.html
fast/events/pagehide-prompt.html
fast/events/unload-alert.html
fast/events/pagehide-alert.html
fast/events/unload-showModalDialog.html
fast/loader/frames-with-unload-handlers-in-page-cache.html

[Build Bot]

Created attachment 265132 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Geoffrey Garen]

Forgot to update test results after changing console message.

[Geoffrey Garen]

Created attachment 265202 [details]
Patch

[Chris Dumez]

Comment on attachment 265202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265202&action=review

> LayoutTests/compositing/iframes/iframe-size-from-zero-expected.txt:35
> +                                  (contentsOpaque 1)

Seems unrelated?

> LayoutTests/fast/dom/Geolocation/page-reload-cancel-permission-requests-expected.txt:1
> +CONSOLE MESSAGE: line 13: TypeError: testRunner.numberOfPendingGeolocationPermissionRequests is not a function. (In 'testRunner.numberOfPendingGeolocationPermissionRequests()', 'testRunner.numberOfPendingGeolocationPermissionRequests' is undefined)

This seems odd (unrelated?).

> LayoutTests/fast/events/attempt-scroll-with-no-scrollbars-expected.txt:1
> +FAILED - window should not have been scrolled but was scrolled to 38

This seems odd (unrelated?).

> LayoutTests/fast/events/context-nodrag-expected.txt:6
> +FAIL window.getSelection().isCollapsed should be true. Was false.

This seems odd.

> LayoutTests/fast/events/drag-and-drop-link-into-focused-contenteditable-expected.txt:-6
> -Test Link

This seems odd (unrelated?).

> LayoutTests/fast/events/force-click-text-selection-behavior-expected.txt:3
> +Failed. This test must be run in the test harness. To run the test manually, select text on the page and then force click a different piece of text. The original selection should not be dismissed.

This seems odd (unrelated?).

> LayoutTests/fast/events/mouseout-on-window-expected.txt:2
> +FAIL: No mouseout was sent.

This seems odd (unrelated?).

> LayoutTests/fast/loader/recursive-before-unload-crash-expected.txt:-1
> -This test demonstrates a problem with our handling of the beforeunload event.

Removes the expected result but keeps the test?

> LayoutTests/platform/mac/platform/mac/fast/loader/file-url-mimetypes-2-expected.txt:14
> +example-m3u.m3u has MIME type audio/mpegurl

This seems odd (unrelated?).

> LayoutTests/platform/mac/platform/mac/fast/loader/file-url-mimetypes-2-expected.txt:121
> +example-rpm.rpm has MIME type audio/x-pn-realaudio-plugin

This seems odd (unrelated?).

[Geoffrey Garen]

Created attachment 265206 [details]
Patch

[Geoffrey Garen]

Removed accidental test result changes.

[Chris Dumez]

Comment on attachment 265206 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265206&action=review

r=me with comments.

> Source/WebCore/loader/FrameLoader.cpp:2822
> +void FrameLoader::handleUnloadEvents(UnloadEventPolicy unloadEventPolicy)

I think 'handle' is a bit vague, maybe 'dispatch' would be clearer?

> Source/WebCore/loader/FrameLoader.cpp:2825
> +    Page* page = m_frame.page();

nit: I like auto* for these things.

> Source/WebCore/loader/FrameLoader.cpp:2827
> +        page->forbidPrompts();

nit: I think it'd be nicer to have an object that calls forbidPrompts() in its constructor and allowPrompts() in its destructor. That object's constructor could take a Page* in parameter and take care of doing the null checking for us as well.
We could ForbiddenPromptsScope or similar.

> Source/WebCore/loader/FrameLoader.cpp:2829
> +    if (m_frame.document()) {

It looks like this whole method does nothing if m_frame.document() is null so I think we can return early here.

> Source/WebCore/loader/FrameLoader.cpp:2844
> +                    RefPtr<Event> unloadEvent(Event::create(eventNames().unloadEvent, false, false));

Ref<>

[Geoffrey Garen]

> > Source/WebCore/loader/FrameLoader.cpp:2822
> > +void FrameLoader::handleUnloadEvents(UnloadEventPolicy unloadEventPolicy)
> 
> I think 'handle' is a bit vague, maybe 'dispatch' would be clearer?

I agree. I'll make this change in a follow-up patch, since other functions in the class and related classes use "handle" for load and beforeunload, and I don't want to create inconsistency.

> > Source/WebCore/loader/FrameLoader.cpp:2825
> > +    Page* page = m_frame.page();
> 
> nit: I like auto* for these things.

OK.

> > Source/WebCore/loader/FrameLoader.cpp:2827
> > +        page->forbidPrompts();
> 
> nit: I think it'd be nicer to have an object that calls forbidPrompts() in
> its constructor and allowPrompts() in its destructor. That object's
> constructor could take a Page* in parameter and take care of doing the null
> checking for us as well.
> We could ForbiddenPromptsScope or similar.

OK.

> 
> > Source/WebCore/loader/FrameLoader.cpp:2829
> > +    if (m_frame.document()) {
> 
> It looks like this whole method does nothing if m_frame.document() is null
> so I think we can return early here.

OK.

> > Source/WebCore/loader/FrameLoader.cpp:2844
> > +                    RefPtr<Event> unloadEvent(Event::create(eventNames().unloadEvent, false, false));
> 
> Ref<>

OK.

[Geoffrey Garen]

> > > Source/WebCore/loader/FrameLoader.cpp:2844
> > > +                    RefPtr<Event> unloadEvent(Event::create(eventNames().unloadEvent, false, false));
> > 
> > Ref<>
> 
> OK.

Removed this change because dispatchEvent requires PassRefPtr.

[Geoffrey Garen]

Committed r192270: <http://trac.webkit.org/changeset/192270>