Bug 93078 - Improve SharedBuffer testing
Summary: Improve SharedBuffer testing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dongseong Hwang
URL:
Keywords: InRadar
Depends on:
Blocks: 155739
  Show dependency treegraph
 
Reported: 2012-08-03 01:17 PDT by Dongseong Hwang
Modified: 2016-03-21 17:36 PDT (History)
14 users (show)

See Also:


Attachments
Patch (2.22 KB, patch)
2012-08-03 01:20 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (2.36 KB, patch)
2012-08-04 00:05 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (2.33 KB, patch)
2012-08-09 04:51 PDT, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (2.37 KB, patch)
2013-01-14 01:17 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (7.35 KB, patch)
2016-03-21 14:56 PDT, Brent Fulgham
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2012-08-03 01:17:20 PDT
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.
Comment 1 Dongseong Hwang 2012-08-03 01:20:10 PDT
Created attachment 156284 [details]
Patch
Comment 2 Alexey Proskuryakov 2012-08-03 09:49:50 PDT
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.
Comment 3 Dongseong Hwang 2012-08-03 21:54:43 PDT
(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?
Comment 4 Alexey Proskuryakov 2012-08-03 22:12:36 PDT
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?
Comment 5 Dongseong Hwang 2012-08-03 22:40:23 PDT
(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.
Comment 6 Alexey Proskuryakov 2012-08-03 23:53:51 PDT
You haven't mentioned it before. Please give this as a reason why this cannot be changed in ChangeLog.
Comment 7 Dongseong Hwang 2012-08-04 00:05:15 PDT
Created attachment 156520 [details]
Patch
Comment 8 Dongseong Hwang 2012-08-04 00:15:42 PDT
(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.
Comment 9 Dongseong Hwang 2012-08-09 04:51:09 PDT
Created attachment 157432 [details]
Patch
Comment 10 Dongseong Hwang 2012-08-09 04:52:43 PDT
(In reply to comment #9)
> Created an attachment (id=157432) [details]
> Patch

Remove the invalid assertion that I wrote in the previous patch.
Comment 11 Xiaoming Shi 2013-01-14 00:06:30 PST
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;
Comment 12 Dongseong Hwang 2013-01-14 01:17:35 PST
Created attachment 182524 [details]
Patch
Comment 13 Dongseong Hwang 2013-01-14 01:20:10 PST
(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.
Comment 14 Dongseong Hwang 2013-01-14 01:23:01 PST
ping. We are waiting for reivew :)
Comment 15 Xiaoming Shi 2013-01-15 11:26:56 PST
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.
Comment 16 Alexey Proskuryakov 2013-01-18 10:02:53 PST
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.
Comment 17 Dongseong Hwang 2013-01-18 18:54:32 PST
(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"
Comment 18 Alexey Proskuryakov 2013-01-18 20:30:20 PST
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.
Comment 19 Brady Eidson 2013-01-18 23:45:46 PST
(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.
Comment 20 Dongseong Hwang 2013-01-19 00:23:22 PST
(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.
Comment 21 Alexey Proskuryakov 2013-01-19 00:31:18 PST
> 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.
Comment 22 Ryosuke Niwa 2013-07-23 17:07:08 PDT
It seems like this got fixed in Blink: https://chromium.googlesource.com/chromium/blink/+/b69f2c01cf57f3a699006fbd692c70bd176946e4
Comment 23 Radar WebKit Bug Importer 2016-03-21 14:53:38 PDT
<rdar://problem/25277829>
Comment 24 Brent Fulgham 2016-03-21 14:56:33 PDT
Created attachment 274624 [details]
Patch
Comment 25 Brent Fulgham 2016-03-21 14:58:11 PDT
(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.
Comment 26 Ryosuke Niwa 2016-03-21 15:31:49 PDT
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?
Comment 27 Brent Fulgham 2016-03-21 17:06:54 PDT
Committed r198507: <http://trac.webkit.org/changeset/198507>