Bug 186468 – Reload the Web view in case of crash if the client does not implement webViewWebContentProcessDidTerminate delegate

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 342376 [details]
Patch

[EWS Watchlist]

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

[Michael Catanzaro]

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).

[Geoffrey Garen]

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.

[Chris Dumez]

Created attachment 342390 [details]
Patch

[Chris Dumez]

Now mimicking Safari behavior.

[Geoffrey Garen]

> 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.

[Geoffrey Garen]

Comment on attachment 342390 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 342390 [details]
Patch

Clearing flags on attachment: 342390

Committed r232668: <https://trac.webkit.org/changeset/232668>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40987290>