[webkit-reviews] review denied: [Bug 135069] Sharing SharedBuffer between WebCore and ImageIO is racy and crash prone : [Attachment 235306] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jul 22 15:29:12 PDT 2014
Simon Fraser (smfr) <simon.fraser at apple.com> has denied Pratik Solanki
<psolanki at apple.com>'s request for review:
Bug 135069: Sharing SharedBuffer between WebCore and ImageIO is racy and crash
prone
https://bugs.webkit.org/show_bug.cgi?id=135069
Attachment 235306: Patch
https://bugs.webkit.org/attachment.cgi?id=235306&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235306&action=review
I would prefer if the behavior was made very explicit, rather than the implicit
"copy on write".
> Source/WebCore/platform/SharedBuffer.cpp:426
> + if (!m_buffer->hasOneRef() && m_size > m_buffer->data.capacity()) {
> + InternalBuffer* newBuffer = new InternalBuffer;
> + newBuffer->data.reserveInitialCapacity(m_size);
> + newBuffer->data = m_buffer->data;
> + m_buffer = adoptRef(newBuffer);
> + }
I would prefer a very explicit function call which says what we're doing here.
Like replaceInternalBuffer() or something.
> Source/WebCore/platform/SharedBuffer.cpp:432
> + if (!m_buffer->hasOneRef())
Is hasOneRef the right thing to use? What if the other thread has two refs to
its buffer? Can we make the "this is no longer shared" much more explicit?
> Source/WebCore/platform/SharedBuffer.cpp:465
> + InternalBuffer* newBuffer = new InternalBuffer;
> + newBuffer->data.reserveInitialCapacity(m_size);
> + newBuffer->data = m_buffer->data;
> + m_buffer = adoptRef(newBuffer);
This looks familiar.
> Source/WebCore/platform/SharedBuffer.h:177
> + struct InternalBuffer : public RefCounted<InternalBuffer> {
It's not an "internal buffer" if this is public :\
I'd call it RefCountedBuffer or just DataBuffer or something.
More information about the webkit-reviews
mailing list