Bug 150980 - alert, confirm, prompt, showModalDialog should be forbidden during page close and navigation
Summary: alert, confirm, prompt, showModalDialog should be forbidden during page close...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-11-06 15:26 PST by Geoffrey Garen
Modified: 2016-01-19 15:01 PST (History)
9 users (show)

See Also:


Attachments
Patch (37.80 KB, patch)
2015-11-06 15:37 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (45.23 KB, patch)
2015-11-06 15:49 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (45.23 KB, patch)
2015-11-09 16:41 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (975.21 KB, application/zip)
2015-11-09 17:28 PST, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-mavericks (794.73 KB, application/zip)
2015-11-09 17:33 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (927.01 KB, application/zip)
2015-11-09 17:59 PST, Build Bot
no flags Details
Patch (52.33 KB, patch)
2015-11-10 11:04 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (45.48 KB, patch)
2015-11-10 11:36 PST, Geoffrey Garen
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2015-11-06 15:26:48 PST
alert, confirm, prompt, showModalDialog should be forbidden during page close and navigation
Comment 1 Geoffrey Garen 2015-11-06 15:37:36 PST
Created attachment 264965 [details]
Patch
Comment 2 Geoffrey Garen 2015-11-06 15:49:39 PST
Created attachment 264967 [details]
Patch
Comment 3 Geoffrey Garen 2015-11-06 15:51:44 PST
<rdar://problem/23435543>
Comment 4 Brady Eidson 2015-11-06 21:20:06 PST
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.
Comment 5 Geoffrey Garen 2015-11-07 12:46:26 PST
> Perhaps "Use of window.print is not allowed while unloading a page"

OK.
Comment 6 Geoffrey Garen 2015-11-09 16:41:22 PST
Created attachment 265118 [details]
Patch
Comment 7 Build Bot 2015-11-09 17:28:41 PST
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
Comment 8 Build Bot 2015-11-09 17:28:44 PST
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
Comment 9 Build Bot 2015-11-09 17:33:56 PST
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
Comment 10 Build Bot 2015-11-09 17:33:59 PST
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
Comment 11 Build Bot 2015-11-09 17:59:44 PST
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
Comment 12 Build Bot 2015-11-09 17:59:47 PST
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
Comment 13 Geoffrey Garen 2015-11-09 21:06:17 PST
Forgot to update test results after changing console message.
Comment 14 Geoffrey Garen 2015-11-10 11:04:59 PST
Created attachment 265202 [details]
Patch
Comment 15 Chris Dumez 2015-11-10 11:16:04 PST
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?).
Comment 16 Geoffrey Garen 2015-11-10 11:36:13 PST
Created attachment 265206 [details]
Patch
Comment 17 Geoffrey Garen 2015-11-10 11:36:59 PST
Removed accidental test result changes.
Comment 18 Chris Dumez 2015-11-10 12:47:39 PST
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<>
Comment 19 Geoffrey Garen 2015-11-10 14:07:04 PST
> > 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.
Comment 20 Geoffrey Garen 2015-11-10 14:07:31 PST
> > > Source/WebCore/loader/FrameLoader.cpp:2844
> > > +                    RefPtr<Event> unloadEvent(Event::create(eventNames().unloadEvent, false, false));
> > 
> > Ref<>
> 
> OK.

Removed this change because dispatchEvent requires PassRefPtr.
Comment 21 Geoffrey Garen 2015-11-10 14:19:41 PST
Committed r192270: <http://trac.webkit.org/changeset/192270>