Crash in HistoryController::updateForCommit dereferencing a null HistoryItem

• Component: Page Loading
• Status: RESOLVED FIXED

[Brady Eidson]

We're seeing crashes in HistoryController::updateForCommit dereferencing a null HistoryItem

      38 WebCore: WebCore::HistoryController::updateForCommit() <==
        30 WebCore: WebCore::HistoryController::updateForCommit()
        | 30 WebCore: WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*)
        |   30 WebCore: WebCore::FrameLoader::commitProvisionalLoad()
        |     30 WebCore: WebCore::DocumentLoader::commitLoad(char const*, int)
        |       28 WebCore: WebCore::CachedRawResource::notifyClientsDataWasReceived(char const*, unsigned int)
        |       | 28 WebCore: WebCore::CachedRawResource::addDataBuffer(WebCore::SharedBuffer&)
        |       |   28 WebCore: WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::PassRefPtr<WebCore::SharedBuffer>, long long, WebCore::DataPayloadType)
        |       |     28 WebCore: WebCore::SubresourceLoader::didReceiveData(char const*, unsigned int, long long, WebCore::DataPayloadType)
        |       |       28 WebKit: WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&)


The problem is when setCurrentItem(*m_provisionalItem) executes, but m_provisionalItem is null.

In updateForCommit there's been a long standing "ASSERT(m_provisionalItem)", but it is still definitely null sometimes. For many years, setting the current item to null in release builds has been a thing.

As of http://trac.webkit.org/changeset/179472 we stopped successfully handing the nullptr and started crashing instead.

While we want to get to the bottom of why the provisional item might be null (which is why we have the ASSERT there), we should restore the previous "handles null" behavior to get rid of the crashes.

<rdar://problem/21371589>

[Brady Eidson]

Created attachment 256593 [details]
Patch v1

[Chris Dumez]

Comment on attachment 256593 [details]
Patch v1

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

> Source/WebCore/loader/FrameLoader.cpp:3184
> +    history().setCurrentItem(&item);

Here it cannot be null.

> Source/WebCore/loader/HistoryController.cpp:474
> +        setCurrentItem(m_provisionalItem.get());

Here, we would hit the assert on the previous line if it were null.

> Source/WebCore/loader/HistoryController.cpp:515
> +        setCurrentItem(m_provisionalItem.get());

Here, we already made sure it isn't null at the beginning of the method.

> Source/WebCore/loader/HistoryController.cpp:564
> +    setCurrentItem(m_provisionalItem.get());

Here, we already made sure it isn't null at the beginning of the method.

> Source/WebCore/loader/HistoryController.cpp:667
> +    setCurrentItem(item.ptr());

Here it cannot be null.

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 256593 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256593&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:3184
> > +    history().setCurrentItem(&item);
> 
> Here it cannot be null.

I know.

> > Source/WebCore/loader/HistoryController.cpp:515
> > +        setCurrentItem(m_provisionalItem.get());
> 
> Here, we already made sure it isn't null at the beginning of the method.

I know.
 
> > Source/WebCore/loader/HistoryController.cpp:564
> > +    setCurrentItem(m_provisionalItem.get());
> 
> Here, we already made sure it isn't null at the beginning of the method.

I know.

> > Source/WebCore/loader/HistoryController.cpp:667
> > +    setCurrentItem(item.ptr());
> 
> Here it cannot be null.

I know.

> > Source/WebCore/loader/HistoryController.cpp:474
> > +        setCurrentItem(m_provisionalItem.get());
> 
> Here, we would hit the assert on the previous line if it were null.

Yes, which is exactly what I described in the problem Description. But in release builds there is no ASSERT and we dereference null.

This is not theoretical - It's definitely happening in release builds a non-trivial amount.

[Chris Dumez]

Comment on attachment 256593 [details]
Patch v1

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

r=me to get rid of the crash short-term but it may be papering over a real bug :/

>>> Source/WebCore/loader/HistoryController.cpp:474
>>> +        setCurrentItem(m_provisionalItem.get());
>> 
>> Here, we would hit the assert on the previous line if it were null.
> 
> Yes, which is exactly what I described in the problem Description. But in release builds there is no ASSERT and we dereference null.
> 
> This is not theoretical - It's definitely happening in release builds a non-trivial amount.

Then, I think we should add a FIXME comment indicating that our assertion above doesn't always hold true and that it should be investigated why.

[Brady Eidson]

(In reply to comment #4)
> Comment on attachment 256593 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=256593&action=review
> 
> r=me to get rid of the crash short-term but it may be papering over a real
> bug :/

There's definitely a real bug, and it's existed since at least 2009 without revealing itself. (That's when the assert was added)

> >>> Source/WebCore/loader/HistoryController.cpp:474
> >>> +        setCurrentItem(m_provisionalItem.get());
> >> 
> >> Here, we would hit the assert on the previous line if it were null.
> > 
> > Yes, which is exactly what I described in the problem Description. But in release builds there is no ASSERT and we dereference null.
> > 
> > This is not theoretical - It's definitely happening in release builds a non-trivial amount.
> 
> Then, I think we should add a FIXME comment indicating that our assertion
> above doesn't always hold true and that it should be investigated why.

K.

[Brady Eidson]

https://trac.webkit.org/changeset/186683