Bug 93078 – Improve SharedBuffer testing

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Dongseong Hwang]

After SharedBuffer::copy(), SharedBuffer::append() often causes segmentation fault,
because copy() calls clone->m_buffer.append(m_segments[i], segmentSize) even if
'i' is the last index. The data size of m_segments.last() is often less than
segmentSize. So, in the cloned instance m_size < (m_buffer.size() + SUM(m_segments[i].size())).
This patch appends the exact size of the last segment instead of segmentSize.

[Dongseong Hwang]

Created attachment 156284 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 156284 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        No new tests - no new testable functionality.

This doesn't make sense to me. It's not just new functionality that we require tests for, we definitely require tests for crash fixes too.

[Dongseong Hwang]

(In reply to comment #2)
> (From update of attachment 156284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156284&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        No new tests - no new testable functionality.
> 
> This doesn't make sense to me. It's not just new functionality that we require tests for, we definitely require tests for crash fixes too.

Thank you for your advice.

Could you explain how I can test this crash fix? Or is it ok to change the changelog?

[Alexey Proskuryakov]

As you mentioned in bug description, this issue often causes segmentation faults. To create a regression test, you need to simulate the conditions in which the crash occurs.

Are you seeing that on a live web site?

[Dongseong Hwang]

(In reply to comment #4)
> As you mentioned in bug description, this issue often causes segmentation faults. To create a regression test, you need to simulate the conditions in which the crash occurs.
> 
> Are you seeing that on a live web site?

No, currently there are only two sites that use SharedBuffer::copy(): ImageDocument::finish() and IconDatabase::setIconDataForIconURL().
It seems those cases do not use append after copy.

I found this bug when I implemented parallel image decoder (Bug 90375).
As I mentioned, I often encountered segmentation fault when running parallel image decoder, because it uses copy and append.

I've tried to make a test, but I'm not sure how to test it.

[Alexey Proskuryakov]

You haven't mentioned it before. Please give this as a reason why this cannot be changed in ChangeLog.

[Dongseong Hwang]

Created attachment 156520 [details]
Patch

[Dongseong Hwang]

(In reply to comment #6)
> You haven't mentioned it before. Please give this as a reason why this cannot be changed in ChangeLog.

I'm sorry if the changelog was confusing. I submitted a new patch after amending the changelog.

[Dongseong Hwang]

Created attachment 157432 [details]
Patch

[Dongseong Hwang]

(In reply to comment #9)
> Created an attachment (id=157432) [details]
> Patch

Remove the invalid assertion that I wrote in the previous patch.

[Xiaoming Shi]

I have also hit this issue this week. Just wonder when this patch will be committed.

Also, the line in the patch:

+        unsigned positionInSegment = offsetInSegment(m_size - m_buffer.size());

will make positionInSegment to 0 if the size of the last segment is segmentSize. You can use:

unsigned positionInSegment = m_size - m_buffer.size() - secondToLast * segmentSize;

[Dongseong Hwang]

Created attachment 182524 [details]
Patch

[Dongseong Hwang]

(In reply to comment #11)
> I have also hit this issue this week. Just wonder when this patch will be committed.

Could you explain how you encounter this bug. reviewers needs layout tests but I can not make test as I mentioned changelog.

> unsigned positionInSegment = m_size - m_buffer.size() - secondToLast * segmentSize;

you're right! thank you.

[Dongseong Hwang]

ping. We are waiting for reivew :)

[Xiaoming Shi]

I encountered this bug when I was debugging WebKit, where I cloned a SharedBuffer and later compared it with the original one. So I cannot provide a test case either.

[Alexey Proskuryakov]

Comment on attachment 182524 [details]
Patch

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

I've been asked to review this patch, but I cannot figure out the relationship between the many data members in this class. It has m_buffer, m_segments, m_purgeableBuffer, m_dataArray, m_cfData. Which of these can be non-empty simultaneously? This class doesn't even have assertions to ensure such invariants at runtime!

What does it mean that the buffer is "shared"?

It appears that copy() should be doing more or less the same thing as data(), but the implementations have nothing in common.

I suspect that there are bigger design issues with this class than just this crash in an unused code path. It needs to be clarified how this class is expected to be used via better design.

> Source/WebCore/ChangeLog:8
> +        Internal review by Jae Hyun Park.

This comment is unhelpful. What is internal review? Do reviewers or people reading ChangeLogs need to know who performed it?

At the very least, please clarify such comments in the future (maybe "Company100 internal review by Jae Hyun Park")? Or better yet, just omit them.

[Dongseong Hwang]

(In reply to comment #16)
> (From update of attachment 182524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182524&action=review

Your opinion is valuable, but I'd like to little bit more focus on this patch.

> I've been asked to review this patch, but I cannot figure out the relationship between the many data members in this class. It has m_buffer, m_segments, m_purgeableBuffer, m_dataArray, m_cfData. Which of these can be non-empty simultaneously? This class doesn't even have assertions to ensure such invariants at runtime!

I agree.  Only m_buffer and m_segments are necessary.
m_purgeableBuffer, m_dataArray, m_cfData are memory optimization for only Apple AFAIK. I wish we will refactor this class more succinct later.

> What does it mean that the buffer is "shared"?

This class's main purpose is to store network data. For example, we receive image date via many packet. SharedBuffer stores data in many packet in not-continuous memory.
We can rename this class later if we think 'shared' does not make sense.
 
> It appears that copy() should be doing more or less the same thing as data(), but the implementations have nothing in common.

data() and copy() are different.
data() moves all data into continuous memory and returns the first pointer of buffer. for example, many image decoder library needs continuous data.
copy() is clone. copy() creates new SharedBuffer. Two SharedBuffer does not share one real buffer.

> I suspect that there are bigger design issues with this class than just this crash in an unused code path. It needs to be clarified how this class is expected to be used via better design.

I wish we will execute your opinion later as well as I wish this patch land. Some people are waiting.


> At the very least, please clarify such comments in the future (maybe "Company100 internal review by Jae Hyun Park")? Or better yet, just omit them.

My company needs to give credit all contributors of the patch. I'll rephrase "Company100 internal review by Jae Hyun Park"

[Alexey Proskuryakov]

Comment on attachment 182524 [details]
Patch

I should have actually marked this reviewed. Making semi-arbitatry modifications to support use cases that do not exist in WebKit truck is not the right way forward.

[Brady Eidson]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 182524 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=182524&action=review
> 
> Your opinion is valuable, but I'd like to little bit more focus on this patch.

Everything Alexey said was directly related to his review of this patch.

> > I've been asked to review this patch, but I cannot figure out the relationship between the many data members in this class. It has m_buffer, m_segments, m_purgeableBuffer, m_dataArray, m_cfData. Which of these can be non-empty simultaneously? This class doesn't even have assertions to ensure such invariants at runtime!
> 
> I agree.  Only m_buffer and m_segments are necessary.
> m_purgeableBuffer, m_dataArray, m_cfData are memory optimization for only Apple AFAIK. I wish we will refactor this class more succinct later.

Apple introduced this class and Apple did most of the initial work on it.  Removing Apple performance optimizations would cause a regression in a tested port and would not be allowed.

Alexey was not saying "this class is too complicated and needs to be simplified."  He was just stating "this class is complicated and it is not at all certain what invariants exist.  There should be ASSERTs at the very least to enforce invariants that do exist."

> > What does it mean that the buffer is "shared"?
> 
> This class's main purpose is to store network data. For example, we receive image date via many packet. SharedBuffer stores data in many packet in not-continuous memory.
> We can rename this class later if we think 'shared' does not make sense.

This is simply wrong, and is a misunderstanding of a fundamental class in WebCore.

SharedBuffer was introduced in http://trac.webkit.org/changeset/18621

Back before SharedBuffer it was introduced, we referred to *contiguous* blobs of data by char* (unsafe), Vector<char> (slow, as the full buffer was copied a lot), or platform specific buffers such as NSdata (unfortunate, because it was platform specific)

The original purpose behind SharedBuffer was to make a RefCounted contiguous buffer that was platform independent.  It had the ability from day one to wrap platform buffers, though, for efficiencies sake.

SharedBuffer picked up the ability to wrap non-contiguous buffers later.  

> > It appears that copy() should be doing more or less the same thing as data(), but the implementations have nothing in common.
> 
> data() and copy() are different.
> data() moves all data into continuous memory and returns the first pointer of buffer. for example, many image decoder library needs continuous data.
> copy() is clone. copy() creates new SharedBuffer. Two SharedBuffer does not share one real buffer.
> 
> > I suspect that there are bigger design issues with this class than just this crash in an unused code path. It needs to be clarified how this class is expected to be used via better design.
> 
> I wish we will execute your opinion later as well as I wish this patch land. Some people are waiting.

Alexey has been a reviewer in the open source project since before SharedBuffer was introduced.

His opinion is not dismissible just because some people - who appear to be quite new to the project and don't fully appreciate the history of what they are changing - want this to land soon.

As a reviewer, he has said it is not clear to him that this patch is correct.

If the correctness of a patch is called in to question by a reviewer it is the burden of the patch submitter to prove it is correct.

[Dongseong Hwang]

(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (From update of attachment 182524 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=182524&action=review
> If the correctness of a patch is called in to question by a reviewer it is the burden of the patch submitter to prove it is correct.

I fully respect webkit developers' all history and effort. I never think his opinion can be dismissible. If you felt this, it is due to my sentence that can cause misunderstanding easily.
If Alexey and you felt bad, I'm very sorry.

[Alexey Proskuryakov]

> data() and copy() are different.
> data() moves all data into continuous memory and returns the first pointer of buffer. for example, many image decoder library needs continuous data.
> copy() is clone. copy() creates new SharedBuffer. Two SharedBuffer does not share one real buffer.

Both functions build a contiguous buffer, but look at different data members. In particular, copy() ignores m_dataArray.

It's unclear why copy() even strives to build a contiguous buffer, or to avoid sharing any data with the original. This is very inefficient, and is not regular behavior for copying, which could just reference immutable objects (like CFDataRefs). If there is a reason for that, it needs to be clarified through comments and better naming.

[Ryosuke Niwa]

It seems like this got fixed in Blink: https://chromium.googlesource.com/chromium/blink/+/b69f2c01cf57f3a699006fbd692c70bd176946e4

[Radar WebKit Bug Importer]

<rdar://problem/25277829>

[Brent Fulgham]

Created attachment 274624 [details]
Patch

[Brent Fulgham]

(In reply to comment #22)
> It seems like this got fixed in Blink:
> https://chromium.googlesource.com/chromium/blink/+/
> b69f2c01cf57f3a699006fbd692c70bd176946e4

I imported those tests into WebKit ToT (see updated patch for this bug), and I do not see any errors, even under GuardMalloc.

[Ryosuke Niwa]

Comment on attachment 274624 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Add three test cases from the Blink project that cover various append,
> +        copy, and createArrayBuffer calls.

Can we attribute to a Blink commit?

[Brent Fulgham]

Committed r198507: <http://trac.webkit.org/changeset/198507>