Bug 169936

Summary: WebKit should disallow beforeunload alerts from web pages users have never interacted with
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, bfulgham, buildbot, cdumez, darin, dbates, ddkilzer, japhet, rmondello, rniwa, sam, teodor.atroshenko, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://html.spec.whatwg.org/multipage/browsers.html#prompt-to-unload-a-document
Bug Depends on:    
Bug Blocks: 169995    
Attachments:
Description Flags
WIP patch
none
WIP patch
none
Patch
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-03-21 16:38:54 PDT
WebKit should disallow beforeunload alerts from web pages users have never interacted with.
Comment 1 Chris Dumez 2017-03-21 16:39:16 PDT
<rdar://problem/23798897>
Comment 2 Chris Dumez 2017-03-21 16:40:08 PDT
Created attachment 305048 [details]
WIP patch
Comment 3 Chris Dumez 2017-03-21 16:43:07 PDT
https://html.spec.whatwg.org/multipage/browsers.html#prompt-to-unload-a-document (Step 8):
"""
The user agent is encouraged to avoid asking the user for confirmation if it judges that doing so would be annoying, deceptive, or pointless. A simple heuristic might be that if the user has not interacted with the document, the user agent would not ask for confirmation before unloading it.
"""
Comment 4 Chris Dumez 2017-03-21 18:19:04 PDT
Created attachment 305057 [details]
WIP patch
Comment 5 Chris Dumez 2017-03-21 19:02:38 PDT
Created attachment 305063 [details]
Patch
Comment 6 Build Bot 2017-03-21 22:36:51 PDT
Comment on attachment 305063 [details]
Patch

Attachment 305063 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3379827

New failing tests:
fast/events/beforeunload-alert-user-interaction2.html
fast/loader/show-only-one-beforeunload-dialog.html
http/tests/misc/iframe-beforeunload-dialog-matching-ancestor-securityorigin.html
fast/events/beforeunload-alert-user-interaction.html
fast/loader/form-submission-after-beforeunload-cancel.html
http/tests/misc/iframe-beforeunload-dialog-not-matching-ancestor-securityorigin.html
Comment 7 Build Bot 2017-03-21 22:36:53 PDT
Created attachment 305078 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 8 Chris Dumez 2017-03-22 09:13:37 PDT
Created attachment 305101 [details]
Patch
Comment 9 Build Bot 2017-03-22 10:44:35 PDT
Comment on attachment 305101 [details]
Patch

Attachment 305101 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3388119

New failing tests:
http/tests/misc/iframe-beforeunload-dialog-matching-ancestor-securityorigin.html
http/tests/misc/iframe-beforeunload-dialog-not-matching-ancestor-securityorigin.html
fast/loader/form-submission-after-beforeunload-cancel.html
fast/loader/show-only-one-beforeunload-dialog.html
Comment 10 Build Bot 2017-03-22 10:44:38 PDT
Created attachment 305108 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 11 Chris Dumez 2017-03-22 12:50:17 PDT
Created attachment 305116 [details]
Patch
Comment 12 Chris Dumez 2017-03-22 12:56:28 PDT
Created attachment 305118 [details]
Patch
Comment 13 Brent Fulgham 2017-03-22 13:16:35 PDT
Comment on attachment 305118 [details]
Patch

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

Looks good. I had a question about whitespace in the test file, but nothing critical.

> LayoutTests/http/tests/misc/iframe-beforeunload-dialog-not-matching-ancestor-securityorigin.html:16
> +        // Simulate a user interaction with the page so that the beforeunload alert shows.

The indent on this comment looks strange. Is that what you intended? Or did we get a tab in the file?
Comment 14 Chris Dumez 2017-03-22 13:19:21 PDT
Created attachment 305121 [details]
Patch
Comment 15 Chris Dumez 2017-03-22 14:14:28 PDT
Comment on attachment 305121 [details]
Patch

Clearing flags on attachment: 305121

Committed r214277: <http://trac.webkit.org/changeset/214277>
Comment 16 Chris Dumez 2017-03-22 14:14:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 teodor.atroshenko 2020-07-08 17:09:47 PDT
I believe the heuristic needs to include an active Media Capture session. When the user entrusts the site with "permanent" access to the microphone and/or camera, the site no longer requires an interaction before it can launch a Media Capture session (e.g., WebRTC call).

Under the current definition of activity a click or a keypress is required, which consequently means that if the person is participating in a WebRTC call, they can accidentally leave an active call by pressing F5 key, or closing the wrong tab, for example. Furthermore, the presence of onbeforeunload prompt will be inconsistent between sessions (the first time, user might see it because they have muted their audio prior to accidentally closing the tab, then the second time they would not see the prompt, as they haven't interacted with the page, leading them to believe that the site is at fault for not showing the prompt).

If the check for an active Media Capture session doesn't seem sufficient, session duration check may also be added. For example, a minimum session duration of 10 seconds may be used. This would allow users to quickly close the tab if they see content that they don't expect to see, while still preventing accidental reloads that may lead to loss of data. An example of such data loss is a file transfer via RTC Data Channel, when a reload would reset the download progress, potentially leading to irrecoverable loss of data, if for example the transfer was initiated by sender pasting a screenshot from the clipboard.

This proposal is cross-posted on Chromium and Firefox bug trackers.
Comment 18 David Kilzer (:ddkilzer) 2020-07-08 21:08:30 PDT
(In reply to teodor.atroshenko from comment #17)
> I believe the heuristic needs to include an active Media Capture session.
> When the user entrusts the site with "permanent" access to the microphone
> and/or camera, the site no longer requires an interaction before it can
> launch a Media Capture session (e.g., WebRTC call).
> 
> Under the current definition of activity a click or a keypress is required,
> which consequently means that if the person is participating in a WebRTC
> call, they can accidentally leave an active call by pressing F5 key, or
> closing the wrong tab, for example. Furthermore, the presence of
> onbeforeunload prompt will be inconsistent between sessions (the first time,
> user might see it because they have muted their audio prior to accidentally
> closing the tab, then the second time they would not see the prompt, as they
> haven't interacted with the page, leading them to believe that the site is
> at fault for not showing the prompt).
> 
> If the check for an active Media Capture session doesn't seem sufficient,
> session duration check may also be added. For example, a minimum session
> duration of 10 seconds may be used. This would allow users to quickly close
> the tab if they see content that they don't expect to see, while still
> preventing accidental reloads that may lead to loss of data. An example of
> such data loss is a file transfer via RTC Data Channel, when a reload would
> reset the download progress, potentially leading to irrecoverable loss of
> data, if for example the transfer was initiated by sender pasting a
> screenshot from the clipboard.
> 
> This proposal is cross-posted on Chromium and Firefox bug trackers.

Please file a new bug to track this remaining issue, and relate it to this bug.  Thanks!