Bug 189568 – WebPageProxy::reportPageLoadResult can crash on some code paths

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Keith Rollin]

WebPageProxy::reportPageLoadResult (which is called from WebPageProxy::didFinishLoadForFrame) can sometimes crash when accessing m_pageLoadStart (a std::optional) in its unloaded state. Normally, m_pageLoadStart is initialized in WebPageProxy::didStartProvisionalLoadForFrame, which one would expect would be called before WebPageProxy::didFinishLoadForFrame. But that turns out to not always be the case. It's not apparent under what conditions didStartProvisionalLoadForFrame will not be called, but it's happening in the wild, leading to crashes now that std::optional asserts in release builds on bad accesses (see Bug 187669).

rdar://problem/44356683

[Keith Rollin]

Fix this by adding checks of m_pageLoadState before all calls to reportPageLoadResult. The check of m_pageLoadState is made outside of reportPageLoadResult rather than inside of it because of review comments suggesting that they worked better outside and ended up being more efficient (Bug 183221, comment #30).

[Keith Rollin]

Also remove an ASSERT that is no longer needed now that std::optional provides its own.

[Keith Rollin]

Created attachment 349599 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 349599 [details]
Patch

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

r- to fix patch applying to trunk and consider feedback.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3564
> +        if (m_pageLoadStart)

Why not just add an early return to reportPageLoadResult() so that the engineer that adds new calls to it doesn’t have to remember this prerequisite?

Then you can remove all the checks (or add Debug asserts if you want to document the expected state of m_pageLoadStart).

[Chris Dumez]

Comment on attachment 349599 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:3564
>> +        if (m_pageLoadStart)
> 
> Why not just add an early return to reportPageLoadResult() so that the engineer that adds new calls to it doesn’t have to remember this prerequisite?
> 
> Then you can remove all the checks (or add Debug asserts if you want to document the expected state of m_pageLoadStart).

I initially suggested to have the check outside to avoid unnecessarily constructing the ResourceError parameters in some cases. That said, since this is apparently error-prone (proof being this bug), I wouldn't be opposed to moving the check inside anymore.

[Keith Rollin]

OK. Fix is now to check m_pageLoadStart inside of reportPageLoadResult.

[Keith Rollin]

Created attachment 349678 [details]
Patch

[Chris Dumez]

Comment on attachment 349678 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 349678 [details]
Patch

Clearing flags on attachment: 349678

Committed r235992: <https://trac.webkit.org/changeset/235992>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.