Bug 135069 – Sharing SharedBuffer between WebCore and ImageIO is racy and crash prone

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Pratik Solanki]

We currently pass a SharedBuffer to ImageIO by creating a WebCoreSharedBufferData wrapper around it. ImageIO access this buffer on a separate thread. This can lead to race conditions and crashes if the main thread modifies m_buffer (because it is in the middle of loading a large image) while ImageIO is trying to decode the image.

[Pratik Solanki]

<rdar://problem/17470655>

[Pratik Solanki]

Created attachment 235205 [details]
Part 1 of WIP patch

[Pratik Solanki]

Created attachment 235206 [details]
Part 2 of WIP patch

[Pratik Solanki]

Created attachment 235306 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 235306 [details]
Patch

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.

[Pratik Solanki]

(In reply to comment #5)
> (From update of attachment 235306 [details])
> 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".

Ok. 

> > 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.

Good idea.

> > 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?

I think hasOneRef() is the best approach - we could have a helper function that returns m_buffer->hasOneRef() if that would make the code clearer.

> > 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.

DataBuffer it is!

[Pratik Solanki]

Created attachment 235345 [details]
Patch

[Pratik Solanki]

Mac isn't happy with my patch - I am seeing images failing to load. Review comments on the current patch would be nice but this needs more work before it can be checked in.

[Simon Fraser (smfr)]

Comment on attachment 235345 [details]
Patch

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

> Source/WebCore/platform/SharedBuffer.cpp:424
> +    DataBuffer* newBuffer = new DataBuffer;

We normally avoid bare "new" calls; I think you should wrap this in a local RefPtr. and then do m_buffer = newBuffer.release() at the end.

> Source/WebCore/platform/SharedBuffer.h:177
> +    struct DataBuffer : public RefCounted<DataBuffer> {

Does this need to be ThreadSafeRefCounted?

> Source/WebCore/platform/mac/SharedBufferMac.mm:111
> +        return adoptCF((CFDataRef)adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBufferDataBuffer:copiedBuffer.get()]).leakRef());

That's a lot of adopting! I don't get why the intermediate adoptNS() is useful.

[Build Bot]

Comment on attachment 235345 [details]
Patch

Attachment 235345 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6352807233847296

New failing tests:
fast/images/cmyk-jpeg-with-color-profile.html
editing/pasteboard/4989774.html
fast/canvas/image-potential-subsample.html
http/tests/css/vertical-align-baseline-after-image-load-3.html
http/tests/css/vertical-align-baseline-after-image-load-2.html
editing/pasteboard/paste-noscript.html
http/tests/css/vertical-align-baseline-after-image-load.html
fast/replaced/pdf-as-image.html
editing/pasteboard/paste-noscript-xhtml.xhtml
editing/pasteboard/styled-element-markup.html
editing/pasteboard/paste-visible-script.html
fast/images/pdf-as-image-landscape.html
editing/pasteboard/4641033.html
fast/images/animated-gif-restored-from-bfcache.html
fast/images/pdf-as-image.html

[Build Bot]

Created attachment 235372 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 235345 [details]
Patch

Attachment 235345 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6514147680321536

New failing tests:
editing/pasteboard/contenteditable-pre-2.html
editing/pasteboard/5761530-1.html
editing/pasteboard/4944770-2.html
http/tests/css/vertical-align-baseline-after-image-load-2.html
editing/execCommand/find-after-replace.html
editing/selection/4895428-1.html
editing/pasteboard/5780697-2.html
editing/inserting/insert-3786362-fix.html
editing/pasteboard/4944770-1.html
editing/pasteboard/4242293.html
editing/pasteboard/avoid-copying-body-with-background.html
editing/selection/4895428-4.html
editing/execCommand/paste-1.html
editing/inserting/insert-3907422-fix.html
editing/pasteboard/block-wrappers-necessary.html
editing/pasteboard/5601583-1.html
editing/deleting/paste-with-transparent-background-color.html
editing/style/font-family-with-space.html
editing/pasteboard/5075944.html
editing/pasteboard/19644-2.html
editing/pasteboard/5065605.html
editing/pasteboard/cleanup-on-move.html
editing/pasteboard/4922709.html
editing/pasteboard/4989774.html
editing/style/non-inheritable-styles.html
http/tests/css/vertical-align-baseline-after-image-load.html
editing/pasteboard/4242293-1.html
editing/pasteboard/contenteditable-pre.html
editing/pasteboard/5071074.html
editing/pasteboard/4641033.html

[Build Bot]

Created attachment 235387 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Pratik Solanki]

(In reply to comment #9)
> (From update of attachment 235345 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235345&action=review
> 
> > Source/WebCore/platform/SharedBuffer.cpp:424
> > +    DataBuffer* newBuffer = new DataBuffer;
> 
> We normally avoid bare "new" calls; I think you should wrap this in a local RefPtr. and then do m_buffer = newBuffer.release() at the end.

Ok.

> > Source/WebCore/platform/SharedBuffer.h:177
> > +    struct DataBuffer : public RefCounted<DataBuffer> {
> 
> Does this need to be ThreadSafeRefCounted?

I guess that would be better.

> > Source/WebCore/platform/mac/SharedBufferMac.mm:111
> > +        return adoptCF((CFDataRef)adoptNS([[WebCoreSharedBufferData alloc] initWithSharedBufferDataBuffer:copiedBuffer.get()]).leakRef());
> 
> That's a lot of adopting! I don't get why the intermediate adoptNS() is useful.

Yeah I agree. Will get rid of it.

I've fixed the layout test failures. We need to call data() to coalesce the buffer before creating WebcoreSharedBufferData. And createNSData() needs the same code changes as createCFData().

[Pratik Solanki]

Created attachment 235395 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 235395 [details]
Patch

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

> Source/WebCore/platform/SharedBuffer.cpp:81
> +    , m_buffer(adoptRef(new DataBuffer))

We don't pass the size to the DataBuffer?

[Pratik Solanki]

Committed r171526: <http://trac.webkit.org/changeset/171526>