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.
Created attachment 156284 [details] Patch
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.
(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?
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?
(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.
You haven't mentioned it before. Please give this as a reason why this cannot be changed in ChangeLog.
Created attachment 156520 [details] Patch
(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.
Created attachment 157432 [details] Patch
(In reply to comment #9) > Created an attachment (id=157432) [details] > Patch Remove the invalid assertion that I wrote in the previous patch.
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;
Created attachment 182524 [details] Patch
(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.
ping. We are waiting for reivew :)
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 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.
(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 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.
(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.
(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.
> 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.
It seems like this got fixed in Blink: https://chromium.googlesource.com/chromium/blink/+/b69f2c01cf57f3a699006fbd692c70bd176946e4
<rdar://problem/25277829>
Created attachment 274624 [details] Patch
(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 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?
Committed r198507: <http://trac.webkit.org/changeset/198507>