Bug 235615 – REGRESSION(r287684) speedtest.net uses many GB of memory

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

REGRESSION(r287684) speedtest.net uses many GB of memory

[Alex Christensen]

Created attachment 449974 [details]
Patch

[Alex Christensen]

<rdar://problem/87830583>

[Chris Dumez]

Comment on attachment 449974 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        For a reason that is still somewhat mysterious to me, the change to XMLHttpRequest::didReceiveData

Doesn't inspire confidence :)

[Alex Christensen]

Try it and see for yourself.  Before and after, the difference is quite clear.

[Chris Dumez]

(In reply to Alex Christensen from comment #4)
> Try it and see for yourself.  Before and after, the difference is quite
> clear.

Well, I believe you but it'd be good to figure out WHY still. You may be working around a very important bug that will bite us again in the future.

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> (In reply to Alex Christensen from comment #4)
> > Try it and see for yourself.  Before and after, the difference is quite
> > clear.
> 
> Well, I believe you but it'd be good to figure out WHY still. You may be
> working around a very important bug that will bite us again in the future.

Also, r287684 seems to be doing a lot of extra data copying (when allocating those SharedBuffers) unless I am misreading. It is not clear to me we want to keep going in that direction (perf/memory wise). We may want to revert r287684 instead of e.g.

[Jean-Yves Avenard [:jya]]

I don’t think this is the right fix. 

There are no copies/allocation. The memory coming from the network process is a SharedMemory and is wrapped in a SharedBuffer

So the memory would still be allocated to the network process rather than the content process. 

You need to change the ownership. 

Your change by doing a new allocation/copy as a side effect assign the memory to the content process

[Chris Dumez]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> I don’t think this is the right fix. 
> 
> There are no copies/allocation. The memory coming from the network process
> is a SharedMemory and is wrapped in a SharedBuffer

My understanding was that it is only SharedMemory if the data comes from the network cache, not when it comes from the network?

> 
> So the memory would still be allocated to the network process rather than
> the content process. 
> 
> You need to change the ownership. 
> 
> Your change by doing a new allocation/copy as a side effect assign the
> memory to the content process

[Alex Christensen]

To reduce risk for the branch, I'm preparing a revert of r287684 and r287687.

[Jean-Yves Avenard [:jya]]

(In reply to Chris Dumez from comment #8)
> (In reply to Jean-Yves Avenard [:jya] from comment #7)
> > I don’t think this is the right fix. 
> > 
> > There are no copies/allocation. The memory coming from the network process
> > is a SharedMemory and is wrapped in a SharedBuffer
> 
> My understanding was that it is only SharedMemory if the data comes from the
> network cache, not when it comes from the network?

It's all SharedMemory if the data originates from the network process.
Wrapped in a SharedBuffer.

[Jean-Yves Avenard [:jya]]

Comment on attachment 449974 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:1062
> +        m_binaryResponseBuilder.append(buffer.data(), buffer.size());

BTW, this would revert to the behaviour of what was done prior https://commits.webkit.org/r287684.

So while it a memory/performance regression following r287684; it is not a regression compare to what we had before.

[Alex Christensen]

Created attachment 449993 [details]
Patch

[Alex Christensen]

Created attachment 449995 [details]
safari-613-branch-patch

[Jean-Yves Avenard [:jya]]

Created attachment 450003 [details]
Patch

[youenn fablet]

Comment on attachment 450003 [details]
Patch

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

> Source/WebCore/loader/cache/CachedRawResource.cpp:71
> +        auto incrementalData = data.getSomeData(previousDataSize);

This makes SharedBufferDataView::createSharedBuffer() or more generally SharedBuffer created from views potentially harmful.

Unrelated but I wonder whether we need to update m_data (makeContiguous call) in the case of DataBufferingPolicy::DoNotBufferData.
It seems there is a potential to further reduce some memory allocations in that case: XHR, fetch...

[Jean-Yves Avenard [:jya]]

Comment on attachment 450003 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedRawResource.cpp:71
>> +        auto incrementalData = data.getSomeData(previousDataSize);
> 
> This makes SharedBufferDataView::createSharedBuffer() or more generally SharedBuffer created from views potentially harmful.
> 
> Unrelated but I wonder whether we need to update m_data (makeContiguous call) in the case of DataBufferingPolicy::DoNotBufferData.
> It seems there is a potential to further reduce some memory allocations in that case: XHR, fetch...

harmful in what way? it has always behaved that way.

could you open a bug for following for the non-cache side of things?

[youenn fablet]

> harmful in what way? it has always behaved that way.

A SharedBuffer might be much bigger than its size actually tells us.

> could you open a bug for following for the non-cache side of things?

I filed https://bugs.webkit.org/show_bug.cgi?id=235635

[Jean-Yves Avenard [:jya]]

Comment on attachment 450003 [details]
Patch

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

>>> Source/WebCore/loader/cache/CachedRawResource.cpp:71
>>> +        auto incrementalData = data.getSomeData(previousDataSize);
>> 
>> This makes SharedBufferDataView::createSharedBuffer() or more generally SharedBuffer created from views potentially harmful.
>> 
>> Unrelated but I wonder whether we need to update m_data (makeContiguous call) in the case of DataBufferingPolicy::DoNotBufferData.
>> It seems there is a potential to further reduce some memory allocations in that case: XHR, fetch...
> 
> harmful in what way? it has always behaved that way.
> 
> could you open a bug for following for the non-cache side of things?

Actually, I can optimise things right away as m_data is cleared immediately after when data policy is to not cache

[Jean-Yves Avenard [:jya]]

Comment on attachment 450003 [details]
Patch

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

>>>> Source/WebCore/loader/cache/CachedRawResource.cpp:71
>>>> +        auto incrementalData = data.getSomeData(previousDataSize);
>>> 
>>> This makes SharedBufferDataView::createSharedBuffer() or more generally SharedBuffer created from views potentially harmful.
>>> 
>>> Unrelated but I wonder whether we need to update m_data (makeContiguous call) in the case of DataBufferingPolicy::DoNotBufferData.
>>> It seems there is a potential to further reduce some memory allocations in that case: XHR, fetch...
>> 
>> harmful in what way? it has always behaved that way.
>> 
>> could you open a bug for following for the non-cache side of things?
> 
> Actually, I can optimise things right away as m_data is cleared immediately after when data policy is to not cache

on second thoughts, there's so many re-entrancy happening in this area of the code ; I'd prefer to keep it that way for now and look into the matter at a later stage, too scared of side effects

[youenn fablet]

> on second thoughts, there's so many re-entrancy happening in this area of
> the code ; I'd prefer to keep it that way for now and look into the matter
> at a later stage, too scared of side effects

Yep, it would be nice to make that code easier to reason about.

[EWS]

Committed r288614 (246433@main): <https://commits.webkit.org/246433@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450003 [details].

[Jean-Yves Avenard [:jya]]

Comment on attachment 449993 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This effectively reverts r287684 and r287687.

on ToT, you would need to revert several more changes for this test to be green. In particular https://commits.webkit.org/r287808

[Geoffrey Garen]

Comment on attachment 450003 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        However, that caused every single intermediary accumulated buffers to be
> +        kept referenced by the XMLHttpRequest SharedBufferBuilder leading to
> +        massive memory use.

Can you explain this detail a little more? What behavior changes when we operate on 'data' instead of 'm_data'?

[Geoffrey Garen]

Comment on attachment 450003 [details]
Patch

Seems like a valuable thing to test, now that we've discovered these quirks.

Is it possible to write a layout test for this that would jetsam on our iOS testers, or a cross platform test that uses window.internals to check for crazy high memory use?

We can use an http test with a .cgi script to supply a fast/huge XHR response.

[youenn fablet]

> > Source/WebCore/ChangeLog:18
> > +        However, that caused every single intermediary accumulated buffers to be
> > +        kept referenced by the XMLHttpRequest SharedBufferBuilder leading to
> > +        massive memory use.
> 
> Can you explain this detail a little more? What behavior changes when we
> operate on 'data' instead of 'm_data'?

Here is my understanding. For each update, we do the following steps:
- Set m_data to a new big continuous chunk of data that stores all received data
- Create a view of the new data as a SharedBuffer. This SharedBuffer references the big continuous chunk
- XHR stores a ref to the view, hence keep the big chunk alive.

Each XHR chunk, although small in data that can be accessed, is actually keeping in memory all temporary created m_data chunks.

> Is it possible to write a layout test for this that would jetsam on our iOS
> testers, or a cross platform test that uses window.internals to check for
> crazy high memory use?

An internals test is probably a good idea though that might require adding a getter in SharedBuffer to know the actual allocated size.

[Geoffrey Garen]

> An internals test is probably a good idea though that might require adding a
> getter in SharedBuffer to know the actual allocated size.

I was thinking of something coarser like window.internals.isUnderMemoryPressure, or adding a new interface like window.internals.memoryUsage which returned something like getrusage.ru_idrss or something.

[Geoffrey Garen]

> - Create a view of the new data as a SharedBuffer. This SharedBuffer
> references the big continuous chunk
> - XHR stores a ref to the view, hence keep the big chunk alive.

Got it: The key to the bug is that each call to didReceiveData receives a view of a *new* SharedBuffer, and each new SharedBuffer is as big as the whole response; but XHR expected to be passed a buffer that was only as big as the new data, to accumulate in a builder.

It seems like we could improve this interface by passing both the new data and the accumulated data, since CachedRawResource has both. Then XHR wouldn't need to maintain its own response builder.

[youenn fablet]

I am not sure we want to expose the whole data, not all clients need it.
We should actually be able to just pass the new data directly without any special processing in the nobuffer case, m_data would remain null in that case for instance.

[Jean-Yves Avenard [:jya]]

(In reply to Geoffrey Garen from comment #27)
> > - Create a view of the new data as a SharedBuffer. This SharedBuffer
> > references the big continuous chunk
> > - XHR stores a ref to the view, hence keep the big chunk alive.
> 
> Got it: The key to the bug is that each call to didReceiveData receives a
> view of a *new* SharedBuffer, and each new SharedBuffer is as big as the
> whole response; but XHR expected to be passed a buffer that was only as big
> as the new data, to accumulate in a builder.

That's right.
However the key is that the new incoming SharedBuffer is a fragmented one, it's made of multiple chunks, and XHR only expects that last chunk.

The CachedRawResource however maintains a *contiguous* SharedBuffer that is created at each didReceiveData call and that is kept alive for the duration of the loop letting all the client knows about that new chunks.
That contiguous SharedBuffer needs to be kept temporarily because some clients when being notified of a new chunk queries the CachedRawResource through a re-entrant call to get a pointer to the flatten data.

With this bug, the new chunk of data given to the client was a SharedBuffer made from a SharedBufferDataView that kept a strong ref to the temporary flatten data.
As the XHR client keeps a strong reference to every chunks, each accumulated buffer were never deleted.

[Ryan Haddad]

Reverted r288614 for reason:

Caused layout test crashes

Committed r288659 (246467@trunk): <https://commits.webkit.org/246467@trunk>

[Jean-Yves Avenard [:jya]]

Created attachment 450104 [details]
Patch

Fix UAF due to the CachedRawResource being cleared through a re-entrant call

[EWS]

Committed r288667 (246473@main): <https://commits.webkit.org/246473@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450104 [details].