Bug 201727

Created attachment 378659 [details] Patch

This is sometimes crashing in release builds only, i'm starting to suspect the compiler, but in the meantime it's not ready for landing it seems.

This has been here ignored because there isn't any reviewer CC'ed. Next time try to CC any reviewer. You can pass the flag "--suggest-reviewers" to the webkit-patch tool and it will automatically suggest them

This totally makes sense. The introduction of async image decoding is what caused buffers to be accessed from different threads (before that everything happened on the main thread). The patch looks good IMO.

Comment on attachment 378659 [details] Patch Do we only need the refcounting to be thread safe? or also the data can be accessed from multiple threads? because in that case we would need a lock to protect the pixels, no?

(In reply to Carlos Garcia Campos from comment #5) > Comment on attachment 378659 [details] > Patch > > Do we only need the refcounting to be thread safe? or also the data can be > accessed from multiple threads? because in that case we would need a lock to > protect the pixels, no? The buffer is going to be created on the decoding thread and filled there. Once the decoding is finished a repaint is requested and at that point it's when the main thread will start accessing the buffer contents, and the decoding thread shouldn't be accessing it anymore. So I don't think the data can be accessed from both threads at the same time.

(In reply to Miguel Gomez from comment #6) > (In reply to Carlos Garcia Campos from comment #5) > > Comment on attachment 378659 [details] > > Patch > > > > Do we only need the refcounting to be thread safe? or also the data can be > > accessed from multiple threads? because in that case we would need a lock to > > protect the pixels, no? > > The buffer is going to be created on the decoding thread and filled there. > Once the decoding is finished a repaint is requested and at that point it's > when the main thread will start accessing the buffer contents, and the > decoding thread shouldn't be accessing it anymore. So I don't think the data > can be accessed from both threads at the same time. Ok, it doesn't matter much because it's a private helper class, but I think I would rename it as RGBAPixelBufferThreadSafeRefCounted to make it clearer.

Comment on attachment 378659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378659&action=review > Source/WebCore/platform/graphics/ImageBackingStore.h:188 > + class ThreadSafeRGBAPixelBuffer : public ThreadSafeRefCounted<ThreadSafeRGBAPixelBuffer> { As I suggested in the comments I would rename this, because this name is confusing.

Comment on attachment 378659 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=378659&action=review > Source/WebCore/platform/graphics/ImageBackingStore.h:199 > + m_isValid = m_pixels.tryReserveCapacity(initialSize.area().unsafeGet()); This operation on Vector, as name suggests, only reserve-expands the capacity -- it doesn't change the size of it. But the zero-fill in zeroPixelData() does fill the Vector's data based on the size, i.e. it zeroes out nothing. An additional Vector<>::resize() call here (if reservation was successful) would avoid that.

Created attachment 382052 [details] Patch for landing

Created attachment 382053 [details] Patch for landing

Created attachment 382054 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 382054 [details]: imported/w3c/web-platform-tests/css/css-position/position-absolute-container-dynamic-002.html bug 203473 (author: simon.fraser@apple.com) imported/w3c/web-platform-tests/css/css-position/position-absolute-crash-chrome-005.html bug 203474 (author: simon.fraser@apple.com) imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/integrity.html bug 203394 (author: ysuzuki@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 382054 [details] Patch for landing Clearing flags on attachment: 382054 Committed r251651: <https://trac.webkit.org/changeset/251651>

All reviewed patches have been landed. Closing bug.

<rdar://problem/56665041>

I had to revert this locally because it's causing crashes here with my riot ephjy web app (I don't know why it doesn't happen with normal ephy). It's crashing when creating the favicon, see: Thread 1 (Thread 0x7fffeaf03cc0 (LWP 2247)): #0 0x00007ffff53b91f0 in WebCore::ImageBackingStore::image() const () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37 #1 0x00007ffff53a87b1 in WebCore::ScalableImageDecoder::createFrameImageAtIndex(unsigned long, WebCore::SubsamplingLevel, WebCore::DecodingOptions const&) () from /home/cgarcia/gnome/lib/li--Type <RET> for more, q to quit, c to continue without paging-- bwebkit2gtk-4.0.so.37 #2 0x00007ffff4ce65f3 in WebCore::ImageSource::frameAtIndexCacheIfNeeded(unsigned long, WebCore::ImageFrame::Caching, WTF::Optional<WebCore::SubsamplingLevel> const&) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37 #3 0x00007ffff4ce6830 in WebCore::ImageSource::frameImageAtIndexCacheIfNeeded(unsigned long, WebCore::SubsamplingLevel) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37 #4 0x00007ffff4c87297 in WebCore::BitmapImage::frameImageAtIndexCacheIfNeeded(unsigned long, WebCore::SubsamplingLevel, WebCore::GraphicsContext const*) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37 #5 0x00007ffff4c872f3 in WebCore::BitmapImage::nativeImageForCurrentFrame(WebCore::GraphicsContext const*) () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37 #6 0x00007ffff3827d2e in WTF::Detail::CallableWrapper<WebKit::IconDatabase::loadIconForPageURL(WTF::String const&, WebKit::IconDatabase::AllowDatabaseWrite, WTF::CompletionHandler<void (WTF::RefPtr<_cairo_surface, WTF::DumbPtrTraits<_cairo_surface> >&&)>&&)::{lambda()#1}::operator()()::{lambda()#1}, void>::call() () from /home/cgarcia/gnome/lib/libwebkit2gtk-4.0.so.37 #7 0x00007ffff1697f85 in WTF::RunLoop::performWork() () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.0.so.18 #8 0x00007ffff16f4a29 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () from /home/cgarcia/gnome/lib/libjavascriptcoregtk-4.0.so.18 #9 0x00007ffff74a6a1e in g_main_dispatch (context=0x55555567f120) at ../glib/gmain.c:3179 #10 g_main_context_dispatch (context=context@entry=0x55555567f120) at ../glib/gmain.c:3844 #11 0x00007ffff74a6da0 in g_main_context_iterate (context=context@entry=0x55555567f120, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:3917 #12 0x00007ffff74a6e2f in g_main_context_iteration (context=context@entry=0x55555567f120, may_block=may_block@entry=1) at ../glib/gmain.c:3978 #13 0x00007ffff76a9a4d in g_application_run (application=0x55555595c2e0, argc=<optimized out>, argv=<optimized out>) at ../gio/gapplication.c:2559 #14 0x0000555555559bf9 in main (argc=1, argv=0x7fffffffd928) at ../src/ephy-main.c:427

Bots are also exiting early after r251651 due to crashes, so I'll roll this out.

Re-opened since this is blocked by bug 203488

*** Bug 203546 has been marked as a duplicate of this bug. ***

Comment on attachment 382054 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=382054&action=review This patch can be simplified simply using SharedBuffer::DataSegment which is already thread safe ref counted. > Source/WebCore/platform/graphics/ImageBackingStore.h:-205 > - m_pixels = SharedBuffer::create(other.m_pixels->data(), other.m_pixels->size()); > - m_pixelsPtr = reinterpret_cast<RGBA32*>(const_cast<char*>(m_pixels->data())); I think this is the problem, we actually need to copy the data here.

Created attachment 382173 [details] Patch

Comment on attachment 382173 [details] Patch It LGTM, I come to same conclusion about the copy ctor and was about to upload :-), but the DataSegment is much nicer, thanks for taking a look!

Committed r251771: <https://trac.webkit.org/changeset/251771>