Summary: | Reduce indirection in Path / PathStream | ||
---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> |
Component: | Layout and Rendering | Assignee: | Kimmo Kinnunen <kkinnunen> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | bfulgham, cgarcia, simon.fraser, webkit-bug-importer, zalan |
Priority: | P2 | Keywords: | InRadar |
Version: | WebKit Nightly Build | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=263666 |
Description
Chris Dumez
2023-10-04 08:52:54 PDT
Pull request: https://github.com/WebKit/WebKit/pull/18622 Committed 268923@main (4905aa0e3445): <https://commits.webkit.org/268923@main> Reviewed commits have been landed. Closing PR #18622 and removing active labels. I'm seeing crashes now with MotionMark (just loading the page without starting any test) that seem related to this change: #0 0x00007f4bffb79e18 in WebCore::PathStream::copy() const () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1 #1 0x00007f4bffb7bedc in WebCore::Path::ensurePlatformPathImpl() () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1 #2 0x00007f4bffb7c419 in WebCore::Path::platformPath() const () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1 #3 0x00007f4bffbbb575 in WebCore::Cairo::fillPath(WebCore::GraphicsContextCairo&, WebCore::Path const&, WebCore::Cairo::FillSource const&, WebCore::Cairo::ShadowState const&) () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1 #4 0x00007f4bfdceaa90 in Nicosia::PaintingContextCairo::ForPainting::replay(WTF::Vector<std::unique_ptr<Nicosia::PaintingOperation, std::default_delete<Nicosia::PaintingOperation> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&) () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1 #5 0x00007f4bfdcddcd5 in WTF::Detail::CallableWrapper<Nicosia::PaintingEngineThreaded::paint(WebCore::GraphicsLayer&, WTF::Ref<Nicosia::Buffer, WTF::RawPtrTraits<Nicosia::Buffer> >&&, WebCore::IntRect const&, WebCore::IntRect const&, WebCore::IntRect const&, float)::{lambda()#1}, void>::call() () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1 #6 0x00007f4bfdbdf40e in WTF::WorkerPool::Worker::work() () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1 #7 0x00007f4bfdb73053 in WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::{lambda()#1}, void>::call() () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1 #8 0x00007f4bfdba9ab6 in WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1 #9 0x00007f4bfdc10df9 in WTF::wtfThreadEntryPoint(void*) () from /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/libWPEWebKit-2.0.so.1 #10 0x00007f4bfb2a63ec in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:444 #11 0x00007f4bfb326a4c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81 (In reply to Carlos Garcia Campos from comment #4) > I'm seeing crashes now with MotionMark (just loading the page without > starting any test) that seem related to this change: > > #0 0x00007f4bffb79e18 in WebCore::PathStream::copy() const () from > /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/ > libWPEWebKit-2.0.so.1 > #1 0x00007f4bffb7bedc in WebCore::Path::ensurePlatformPathImpl() () from > /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/ > libWPEWebKit-2.0.so.1 > #2 0x00007f4bffb7c419 in WebCore::Path::platformPath() const () from > /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/ > libWPEWebKit-2.0.so.1 > #3 0x00007f4bffbbb575 in > WebCore::Cairo::fillPath(WebCore::GraphicsContextCairo&, WebCore::Path > const&, WebCore::Cairo::FillSource const&, WebCore::Cairo::ShadowState > const&) () > from > /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/ > libWPEWebKit-2.0.so.1 > #4 0x00007f4bfdceaa90 in > Nicosia::PaintingContextCairo::ForPainting::replay(WTF::Vector<std:: > unique_ptr<Nicosia::PaintingOperation, > std::default_delete<Nicosia::PaintingOperation> >, 0ul, > WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&) () from > /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/ > libWPEWebKit-2.0.so.1 > #5 0x00007f4bfdcddcd5 in > WTF::Detail::CallableWrapper<Nicosia::PaintingEngineThreaded::paint(WebCore:: > GraphicsLayer&, WTF::Ref<Nicosia::Buffer, WTF::RawPtrTraits<Nicosia::Buffer> > >&&, WebCore::IntRect const&, WebCore::IntRect const&, WebCore::IntRect > const&, float)::{lambda()#1}, void>::call() () > from > /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/ > libWPEWebKit-2.0.so.1 > #6 0x00007f4bfdbdf40e in WTF::WorkerPool::Worker::work() () from > /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/ > libWPEWebKit-2.0.so.1 > #7 0x00007f4bfdb73053 in > WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker > const&)::{lambda()#1}, void>::call() () > from > /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/ > libWPEWebKit-2.0.so.1 > #8 0x00007f4bfdba9ab6 in > WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) () from > /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/ > libWPEWebKit-2.0.so.1 > #9 0x00007f4bfdc10df9 in WTF::wtfThreadEntryPoint(void*) () from > /home/cgarcia/src/git/gnome/WebKit-WPE-platform/WebKitBuild/Release/lib/ > libWPEWebKit-2.0.so.1 > #10 0x00007f4bfb2a63ec in start_thread (arg=<optimized out>) at > ./nptl/pthread_create.c:444 > #11 0x00007f4bfb326a4c in clone3 () at > ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81 I took a look but it is not immediately clear to me via code inspection what the issue is. Sadly, this is not something I can reproduce as we get no such crash with the Cocoa ports. I did my best to keep PathCairo working in my patch for glib-based ports but I guess I messed up something. If you could provide a bit more debug information, that would really be helpful. It doesn't always happen here, so I think it might be related to threads. I haven't been able to reproduce it with threaded rendering disabled. I didn't expect as path could be used from multiple threads, though but I'm not familiar with that code. (In reply to Carlos Garcia Campos from comment #6) > It doesn't always happen here, so I think it might be related to threads. I > haven't been able to reproduce it with threaded rendering disabled. I didn't > expect as path could be used from multiple threads, though but I'm not > familiar with that code. Oh, maybe this: ``` class PathImpl : public RefCounted<PathImpl> ``` should subclass ThreadSafeRefCounted then? If you can reproduce, even flakily, it'd be worth trying this change I think. (In reply to Chris Dumez from comment #7) > (In reply to Carlos Garcia Campos from comment #6) > > It doesn't always happen here, so I think it might be related to threads. I > > haven't been able to reproduce it with threaded rendering disabled. I didn't > > expect as path could be used from multiple threads, though but I'm not > > familiar with that code. > > Oh, maybe this: > ``` > class PathImpl : public RefCounted<PathImpl> > ``` > > should subclass ThreadSafeRefCounted then? > > If you can reproduce, even flakily, it'd be worth trying this change I think. I haven't been able to reproduce it again with ThreadSafeRefCounted, so I'm confident that fixed it. (In reply to Carlos Garcia Campos from comment #8) > (In reply to Chris Dumez from comment #7) > > (In reply to Carlos Garcia Campos from comment #6) > > > It doesn't always happen here, so I think it might be related to threads. I > > > haven't been able to reproduce it with threaded rendering disabled. I didn't > > > expect as path could be used from multiple threads, though but I'm not > > > familiar with that code. > > > > Oh, maybe this: > > ``` > > class PathImpl : public RefCounted<PathImpl> > > ``` > > > > should subclass ThreadSafeRefCounted then? > > > > If you can reproduce, even flakily, it'd be worth trying this change I think. > > I haven't been able to reproduce it again with ThreadSafeRefCounted, so I'm > confident that fixed it. Would mind doing the PR? Re-opening for pull request https://github.com/apple/WebKit/pull/966 Committed 267815.630@safari-7617-branch (bc765d16baad): <https://commits.webkit.org/267815.630@safari-7617-branch> Reviewed commits have been landed. Closing PR #966 and removing active labels. |