Bug 186468

Summary: Reload the Web view in case of crash if the client does not implement webViewWebContentProcessDidTerminate delegate
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, berto, cgarcia, commit-queue, ews-watchlist, ggaren, gustavo, mcatanzaro, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2018-06-09 17:21:29 PDT
We should attempt to reload the Web view if the client does not implement webViewWebContentProcessDidTerminate delegate. Otherwise, clients not implementing the delegate get a blank view in case of a crash / jetsam.
Comment 1 Chris Dumez 2018-06-09 17:40:27 PDT
Created attachment 342376 [details]
Patch
Comment 2 EWS Watchlist 2018-06-09 17:41:23 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Michael Catanzaro 2018-06-10 09:26:19 PDT
Comment on attachment 342376 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:5883
> +    if (!handledByClient && isCrashOrTerminationDueToProcessLimits(reason))
> +        attemptAutomaticRecoveryAfterCrash();

Hmm, you meant "is crash OR is termination due to process limits" but I read it as "is the reason for crash or termination due to process limits," which is confusing. I was going to complain that it should be !isCrashOrTerminationDueToProcessLimits(reason) because I was confused. Possible alternate names: isCrashOrIsTerminationDueToProcessLimits (clearer but unwieldy) or perhaps doesProcessTerminationReasonAllowAutomaticRecovery (maybe slightly better, also kinda unwieldy).
Comment 4 Geoffrey Garen 2018-06-10 10:33:03 PDT
Comment on attachment 342376 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:5860
> +static bool isCrashOrTerminationDueToProcessLimits(ProcessTerminationReason reason)

How about calling this shouldReloadAfterProcessTermination() and calling the action function WebPageProxy::tryReloadAfterProcessTermination().

> Source/WebKit/UIProcess/WebPageProxy.cpp:5897
> +    if (!m_resetRecentCrashCountTimer.isActive())
> +        m_resetRecentCrashCountTimer.startOneShot(resetRecentCrashCountDelay);

I believe Safari's policy is to reload once per page (or maybe URL? or domain?). I think that policy is preferable. In addition to avoiding crash loops, we want to avoid giving malicious pages an opportunity to retry probabilistic attacks. For example, if ASLR reduces attack success to 1/10, the 20s timer increases attack success to 10/10 after three minutes.
Comment 5 Chris Dumez 2018-06-10 14:26:33 PDT
Created attachment 342390 [details]
Patch
Comment 6 Chris Dumez 2018-06-10 14:26:59 PDT
Now mimicking Safari behavior.
Comment 7 Geoffrey Garen 2018-06-10 14:33:22 PDT
> In addition to avoiding crash
> loops, we want to avoid giving malicious pages an opportunity to retry
> probabilistic attacks.

I was confused about the timer policy. I thought your patch *reloaded* after the timer fired. But that's not true.
Comment 8 Geoffrey Garen 2018-06-10 14:39:56 PDT
Comment on attachment 342390 [details]
Patch

r=me
Comment 9 WebKit Commit Bot 2018-06-10 15:39:28 PDT
Comment on attachment 342390 [details]
Patch

Clearing flags on attachment: 342390

Committed r232668: <https://trac.webkit.org/changeset/232668>
Comment 10 WebKit Commit Bot 2018-06-10 15:39:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-06-10 15:40:21 PDT
<rdar://problem/40987290>