Bug 169142 - Assertion failure in DocumentLoader::dataReceived - NULL 'data' ptr
Summary: Assertion failure in DocumentLoader::dataReceived - NULL 'data' ptr
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-03 12:25 PST by Keith Rollin
Modified: 2020-06-01 14:53 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.52 KB, patch)
2017-03-06 18:51 PST, Keith Rollin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 2017-03-03 12:25:15 PST
In DocumentLoader::dataReceived, there's an ASSERT to make sure the incoming 'data' pointer is not NULL. It gets invoked in this context:

ASSERTION FAILED: data
/Volumes/Data/dev/WebKit/branches/record_playback/OpenSource/Source/WebCore/loader/DocumentLoader.cpp(924) : void WebCore::DocumentLoader::dataReceived(const char *, int)
1   0x10d363e1d WTFCrash
2   0x11036b749 WebCore::DocumentLoader::dataReceived(char const*, int)
3   0x11036bfb4 WebCore::DocumentLoader::dataReceived(WebCore::CachedResource&, char const*, int)
4   0x10fe67a5c WebCore::CachedRawResource::didAddClient(WebCore::CachedResourceClient&)
5   0x10fe70063 WebCore::CachedResource::Callback::timerFired()
...

We are getting here because something is calling CachedResource::addClientToSet. This results in the client being queued up to be added to the set of clients later:

       m_clientsAwaitingCallback.add(&client, std::make_unique<Callback>(*this, client));

This creates the timer, resulting in the client being added later (which is what the backtrace above represents). When it is added, it is done so via CachedRawResource::didAddClient. didAddClient will, in part, execute:

   if (m_data)
       client.dataReceived(*this, m_data->data(), m_data->size());

m_data is a RefPtr<SharedBuffer>. Even though didAddClient checked to see the RefPtr contains a SharedBuffer, it does not check to see if the SharedBuffer references NULL data. In this case, it does, and DocumentLoader::dataReceived complains about that with the ASSERT.

The SharedBuffer contains NULL because of Entry::encodeAsStorageRecord and initializeBufferFromStorageRecord (in NetworkCacheEntry.cpp). The former will cache a network response with:

   Data body;
   if (m_buffer)
       body = { reinterpret_cast<const uint8_t*>(m_buffer->data()), m_buffer->size() };

   return { m_key, m_timeStamp, header, body, { } };

That is, if m_buffer (which is also a RefPtr<WebCore::SharedBuffer>) does not contain a SharedBuffer, it creates and stores an empty Data for the body. Later, in initializeBufferFromStorageRecord, the network response is retrieved from the cache with:

   m_buffer = WebCore::SharedBuffer::create(m_sourceStorageRecord.body.data(), m_sourceStorageRecord.body.size());

This will create a a SharedBuffer that references no data. Its data() will return nullptr and its size() will return zero. This gets passed up the framework until we get the crash above.
Comment 1 Keith Rollin 2017-03-06 18:51:05 PST
If anyone knows why this assertion failure doesn't trigger under regular use, I'd be interested in hearing it.

If anyone knows how to write a test for this, I'm open to that, too.
Comment 2 Keith Rollin 2017-03-06 18:51:55 PST
Created attachment 303601 [details]
Patch
Comment 3 Antti Koivisto 2017-03-08 13:14:37 PST
Comment on attachment 303601 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        No new tests. There is no new functionality. This problem only shows
> +        up when running a new version of the PLT. It hasn't shown up at any
> +        other time.

It should be possible to make a test if we have a repro.
Comment 4 Keith Rollin 2017-03-08 13:19:22 PST
(In reply to comment #3)
> It should be possible to make a test if we have a repro.

Like I ask above: how? Last time I checked, I was able to get this to trigger pretty regularly when running the new PLT mechanism. But I don't know how to turn that into a test.
Comment 5 Keith Rollin 2017-03-20 21:50:01 PDT
Antti, with your being in town, would there be a good time to talk about how to test this?
Comment 6 Keith Rollin 2018-12-04 19:03:02 PST
The conditions under which this issue occurred were removed in bug 192296, so there's nothing to address any more.
Comment 7 Maciej Stachowiak 2020-06-01 14:52:51 PDT
Comment on attachment 303601 [details]
Patch

Unflagging and obsoleting since this was resolved.