Summary: | Reload the Web view in case of crash if the client does not implement webViewWebContentProcessDidTerminate delegate | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | WebKit2 | Assignee: | 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
Chris Dumez
2018-06-09 17:21:29 PDT
Created attachment 342376 [details]
Patch
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 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 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. Created attachment 342390 [details]
Patch
Now mimicking Safari behavior. > 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 on attachment 342390 [details]
Patch
r=me
Comment on attachment 342390 [details] Patch Clearing flags on attachment: 342390 Committed r232668: <https://trac.webkit.org/changeset/232668> All reviewed patches have been landed. Closing bug. |