[webkit-reviews] review granted: [Bug 196548] [ Mac Debug ] TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessForRegularNavigationRetainBundlePage is a flaky crash : [Attachment 367341] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 196548: [ Mac Debug ]
TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessForRegularNavigationRetainBundle
Page is a flaky crash
https://bugs.webkit.org/show_bug.cgi?id=196548

Attachment 367341: Patch

https://bugs.webkit.org/attachment.cgi?id=367341&action=review




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 367341
  --> https://bugs.webkit.org/attachment.cgi?id=367341
Patch

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

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:217
> +    // If the previous provisional load used an existing process, we may
receive leftover IPC for a previous navigation, which we need to ignore.
> +    if (!m_mainFrame || m_mainFrame->frameID() != frameID)
> +	   return;

See below. Could have an overload that takes just a frame ID and not a
navigation ID.

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:226
> +    // If the previous provisional load used an existing process, we may
receive leftover IPC for a previous navigation, which we need to ignore.
> +    if (navigationID != m_navigationID || !m_mainFrame ||
m_mainFrame->frameID() != frameID)
>	   return;

With this idiom repeated so many times, I suggest adding a named member
function that takes navigationID and frameID as arguments. A bonus is that the
function’s name can help document and so we won’t have such repetitive comments
either.

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:347
> +    if (!isMainFrame || (m_mainFrame && m_mainFrame->frameID() != frameID)
|| navigationID != m_navigationID) {

I think we can even refactor this one to use the new helper.