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.
<rdar://problem/17470655>
Created attachment 235205 [details] Part 1 of WIP patch
Created attachment 235206 [details] Part 2 of WIP patch
Created attachment 235306 [details] Patch
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.
(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!
Created attachment 235345 [details] Patch
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.
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.
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
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
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
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
(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().
Created attachment 235395 [details] Patch
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?
Committed r171526: <http://trac.webkit.org/changeset/171526>