We're getting reports of crashes in Resize Observer code: Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Subtype: KERN_INVALID_ADDRESS at 0x0000000000000008 VM Region Info: 0x8 is not in any region. Bytes before following region: 4331192312 REGION TYPE START - END [ VSIZE] PRT/MAX SHRMOD REGION DETAIL UNUSED SPACE AT START ---> __TEXT 000000010228c000-0000000102290000 [ 16K] r-x/r-x SM=COW ...it.WebContent Termination Signal: Segmentation fault: 11 Termination Reason: Namespace SIGNAL, Code 0xb Terminating Process: exc handler [1353] Triggered by Thread: 0 Thread 0 name: Dispatch queue: com.apple.main-thread Thread 0 Crashed: 0 WebCore 0x00000001c2abda8c WebCore::Document::updateResizeObservations(WebCore::Page&) + 156 (WeakPtr.h:64) 1 WebCore 0x00000001c2abdaa4 WebCore::Document::updateResizeObservations(WebCore::Page&) + 180 (Document.cpp:7525) 2 WebCore 0x00000001c2f82d68 WebCore::Page::updateRendering() + 364 (Page.cpp:1313) 3 WebKit 0x00000001c1760318 WebKit::RemoteLayerTreeDrawingArea::flushLayers() + 132 (RemoteLayerTreeDrawingArea.mm:374) 4 WebCore 0x00000001c302e6a4 WebCore::ThreadTimers::sharedTimerFiredInternal() + 216 (ThreadTimers.cpp:129) 5 WebCore 0x00000001c3051644 WebCore::timerFired(__CFRunLoopTimer*, void*) + 28 (MainThreadSharedTimerCF.cpp:74) 6 CoreFoundation 0x00000001ba1bb5b4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 28 (CFRunLoop.c:1757) 7 CoreFoundation 0x00000001ba1bb2f0 __CFRunLoopDoTimer + 880 (CFRunLoop.c:2348) 8 CoreFoundation 0x00000001ba1ba9c0 __CFRunLoopDoTimers + 276 (CFRunLoop.c:2503) 9 CoreFoundation 0x00000001ba1b5afc __CFRunLoopRun + 1920 (CFRunLoop.c:0) 10 CoreFoundation 0x00000001ba1b5054 CFRunLoopRunSpecific + 464 (CFRunLoop.c:3183) 11 Foundation 0x00000001ba4f38c4 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 228 (NSRunLoop.m:374) 12 Foundation 0x00000001ba52d2d4 -[NSRunLoop(NSRunLoop) run] + 88 (NSRunLoop.m:399) 13 libxpc.dylib 0x00000001b9e15360 _xpc_objc_main + 304 (main.m:179) 14 libxpc.dylib 0x00000001b9e17ca0 xpc_main + 148 (init.c:1568) 15 WebKit 0x00000001c184dc6c WebKit::XPCServiceMain(int, char const**) + 360 (XPCServiceMain.mm:147) 16 libdyld.dylib 0x00000001ba040c7c start + 4
rdar://problem/54173715
I'll take a look of this. Do we have the urls of crash pages?
No, we don't have any additional information, sadly.
Here is a more detailed crash call stack: Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000008) Getting symbols for 28C4435F-393E-3FBC-87E7-9CFEBA0FAB85 /System/Library/PrivateFrameworks/WebCore.framework/WebCore... ok [ 0] 0x00000001c2abda8c WebCore`WebCore::Document::updateResizeObservations(WebCore::Page&) [inlined] WebCore::ResizeObserver::WeakValueType* WTF::WeakPtrImpl::get<WebCore::ResizeObserver>() at WeakPtr.h:64:56 0x00000001c2abda7c: cmn x21, #0x1 ; =0x1 0x00000001c2abda80: b.eq 0xe2cb20 ; <+304> [inlined] WebCore::Document::hasSkippedResizeObservations() const at Document.cpp:7559 0x00000001c2abda84: tbnz w8, #0x0, 0xe2cab0 ; <+192> at Document.cpp:7556:14 0x00000001c2abda88: ldr x9, [x23] -> 0x00000001c2abda8c: ldr x8, [x9, #0x8] 0x00000001c2abda90: ldr w10, [x8, #0x4c] 0x00000001c2abda94: cbz w10, 0xe2caa4 ; <+180> [inlined] WebCore::Document::deliverResizeObservations() + 32 at Document.cpp:7555 0x00000001c2abda98: cmp x9, #0x0 ; =0x0 0x00000001c2abda9c: csel x0, xzr, x8, eq [ 0] 0x00000001c2abda8c WebCore`WebCore::Document::updateResizeObservations(WebCore::Page&) [inlined] WTF::WeakPtr<WebCore::ResizeObserver>::get() const + 4 at WeakPtr.h:89 [ 0] 0x00000001c2abda88 WebCore`WebCore::Document::updateResizeObservations(WebCore::Page&) [inlined] WTF::WeakPtr<WebCore::ResizeObserver>::operator->() const at WeakPtr.h:96 [ 0] 0x00000001c2abda88 WebCore`WebCore::Document::updateResizeObservations(WebCore::Page&) [inlined] WebCore::Document::deliverResizeObservations() + 4 at Document.cpp:7523 7519 7520 void Document::deliverResizeObservations() 7521 { 7522 for (const auto& observer : m_resizeObservers) { -> 7523 if (!observer->hasActiveObservations()) 7524 continue; 7525 observer->deliverObservations(); 7526 } 7527 } [ 0] 0x00000001c2abda84 WebCore`WebCore::Document::updateResizeObservations(WebCore::Page&) + 148 at Document.cpp:7555 [ 1] 0x00000001c2abdaa3 WebCore`WebCore::Document::updateResizeObservations(WebCore::Page&) [inlined] WebCore::Document::deliverResizeObservations() + 31 at Document.cpp:7525:19 [ 1] 0x00000001c2abda84 WebCore`WebCore::Document::updateResizeObservations(WebCore::Page&) + 148 at Document.cpp:7555 [ 2] 0x00000001c2f82d67 WebCore`WebCore::Page::updateRendering() + 363 at Page.cpp:1313:19 Getting symbols for DADAF4DE-FBC2-32FF-A641-4CC6D9432DDD /System/Library/Frameworks/WebKit.framework/WebKit... ok [ 3] 0x00000001c1760317 WebKit`WebKit::RemoteLayerTreeDrawingArea::flushLayers() + 131 at RemoteLayerTreeDrawingArea.mm:374:15 [ 4] 0x00000001c302e6a3 WebCore`WebCore::ThreadTimers::sharedTimerFiredInternal() + 215 at ThreadTimers.cpp:129:23 [ 5] 0x00000001c3051643 WebCore`WebCore::timerFired(__CFRunLoopTimer*, void*) + 27 at MainThreadSharedTimerCF.cpp:74:40 Getting symbols for 05D70723-5989-31E9-8633-F71B5D0F2CE1 /System/Library/Frameworks/CoreFoundation.framework/CoreFoundation... ok [ 6] 0x00000001ba1bb5b3 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 27 at CFRunLoop.c:1757:9 [ 7] 0x00000001ba1bb2ef CoreFoundation`__CFRunLoopDoTimer + 879 at CFRunLoop.c:2348:2 [ 8] 0x00000001ba1ba9bf CoreFoundation`__CFRunLoopDoTimers + 275 at CFRunLoop.c:2503:23 [ 9] 0x00000001ba1b5afb CoreFoundation`__CFRunLoopRun + 1919 at CFRunLoop.c:0 [ 10] 0x00000001ba1b5053 CoreFoundation`CFRunLoopRunSpecific + 463 at CFRunLoop.c:3183:18 Getting symbols for 0C953B39-2D12-3FD6-B93A-902C7D918CB8 /System/Library/Frameworks/Foundation.framework/Foundation... ok [ 11] 0x00000001ba4f38c3 Foundation`-[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 227 at NSRunLoop.m:374:5 [ 12] 0x00000001ba52d2d3 Foundation`-[NSRunLoop(NSRunLoop) run] + 87 at NSRunLoop.m:399:12 Getting symbols for 196D3B08-CA14-3696-B592-AA04A85F8BDF /usr/lib/system/libxpc.dylib... ok [ 13] 0x00000001b9e1535f libxpc.dylib`_xpc_objc_main + 303 at main.m:179:3 [ 14] 0x00000001b9e17c9f libxpc.dylib`xpc_main + 147 at init.c:1568:2 [ 15] 0x00000001c184dc6b WebKit`WebKit::XPCServiceMain(int, char const**) + 359 at XPCServiceMain.mm:147:5 Getting symbols for 0C5E6B14-C214-3A21-B8DB-8372DF7297B5 /usr/lib/system/libdyld.dylib... ok [ 16] 0x00000001ba040c7b libdyld.dylib`start + 3 Since m_resizeObservers is a Vector<WeakPtr<ResizeObserver>>, I think it is okay to have one of the pointers to be null. So maybe we need to change the condition in Document::deliverResizeObservations() to be: if (!observer || !observer->hasActiveObservations())
Thanks for the additional information. I guess this crash happens if the active observer is deleted in other observer's callback. There's a test case to reproduce it. I'm working on this.
Created attachment 376375 [details] Test case to reproduce the crash issue
Created attachment 376388 [details] Patch
Hi Simon and Said, This patch is ready to review, please take a look when you can. Thanks:)
Comment on attachment 376388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376388&action=review > Source/WebCore/dom/Document.cpp:7565 > + m_isResizeObserversModified = false; > for (const auto& observer : m_resizeObservers) { The normal way to do this is to copy the vector before enumerating over it. Should we use that approach here?
(In reply to Simon Fraser (smfr) from comment #9) > Comment on attachment 376388 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376388&action=review > > > Source/WebCore/dom/Document.cpp:7565 > > + m_isResizeObserversModified = false; > > for (const auto& observer : m_resizeObservers) { > > The normal way to do this is to copy the vector before enumerating over it. > Should we use that approach here? IIUC, it seems ResizeObservers will always be deleted by GC no matter we copy the vector or not.
Comment on attachment 376388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376388&action=review >>> Source/WebCore/dom/Document.cpp:7565 >>> for (const auto& observer : m_resizeObservers) { >> >> The normal way to do this is to copy the vector before enumerating over it. Should we use that approach here? > > IIUC, it seems ResizeObservers will always be deleted by GC no matter we copy the vector or not. Right, but copy the vector and null-checking the observers should be OK?
Comment on attachment 376388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376388&action=review > Source/WebCore/dom/Document.cpp:7572 > observer->deliverObservations(); I'm not sure I quite understand. The modification to the vector is at here, inside the loop. Do you mean, if we detect the modification, break out the loop, then copy the vector and null-checking the observers?
Comment on attachment 376388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376388&action=review >> Source/WebCore/dom/Document.cpp:7572 >> observer->deliverObservations(); > > I'm not sure I quite understand. > The modification to the vector is at here, inside the loop. > Do you mean, if we detect the modification, break out the loop, then copy the vector and null-checking the observers? No, I mean something like; auto observersToNotify = m_resizeObservers; for (const auto& observer : observersToNotify) { if (observer)...
Comment on attachment 376388 [details] Patch Attachment 376388 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12918674 New failing tests: resize-observer/delete-observers-in-callbacks.html
Created attachment 376403 [details] Archive of layout-test-results from ews113 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 376472 [details] Patch
(In reply to Simon Fraser (smfr) from comment #13) > Comment on attachment 376388 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376388&action=review > > >> Source/WebCore/dom/Document.cpp:7572 > >> observer->deliverObservations(); > > > > I'm not sure I quite understand. > > The modification to the vector is at here, inside the loop. > > Do you mean, if we detect the modification, break out the loop, then copy the vector and null-checking the observers? > > No, I mean something like; > > auto observersToNotify = m_resizeObservers; > for (const auto& observer : observersToNotify) { > if (observer)... Done! Thanks for the explanation:)
Created attachment 376607 [details] Patch
Comment on attachment 376607 [details] Patch Rejecting attachment 376607 [details] from commit-queue. cathiechen@igalia.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 376607 [details] Patch Clearing flags on attachment: 376607 Committed r248830: <https://trac.webkit.org/changeset/248830>
All reviewed patches have been landed. Closing bug.