Bug 200635 – Crash in Document::updateResizeObservations()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Simon Fraser (smfr)]

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

[Simon Fraser (smfr)]

rdar://problem/54173715

[cathiechen]

I'll take a look of this.
Do we have the urls of crash pages?

[Simon Fraser (smfr)]

No, we don't have any additional information, sadly.

[Said Abou-Hallawa]

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())

[cathiechen]

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.

[cathiechen]

Created attachment 376375 [details]
Test case to reproduce the crash issue

[cathiechen]

Created attachment 376388 [details]
Patch

[cathiechen]

Hi Simon and Said,

This patch is ready to review, please take a look when you can.

Thanks:)

[Simon Fraser (smfr)]

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?

[cathiechen]

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

[Simon Fraser (smfr)]

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?

[cathiechen]

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?

[Simon Fraser (smfr)]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[cathiechen]

Created attachment 376472 [details]
Patch

[cathiechen]

(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:)

[cathiechen]

Created attachment 376607 [details]
Patch

[EWS]

Comment on attachment 376607 [details]
Patch

Rejecting attachment 376607 [details] from commit-queue.

[e-mail] 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.

[WebKit Commit Bot]

Comment on attachment 376607 [details]
Patch

Clearing flags on attachment: 376607

Committed r248830: <https://trac.webkit.org/changeset/248830>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.