Bug 211386 – REGRESSION (r261056): [ Mac WK1 ] inspector/console/console-api.html is flaky crashing

• Component: Scrolling
• Status: RESOLVED FIXED

[Truitt Savell]

inspector/console/console-api.html

This test began crashing around r261063

History:
https://results.webkit.org/?suite=layout-tests&test=inspector%2Fconsole%2Fconsole-api.html

Crash Log:
https://build.webkit.org/results/Apple-Catalina-Release-WK1-Tests/r261078%20(5479)/inspector/console/console-api-crash-log.txt

[Ryan Haddad]

Application Specific Backtrace 1:
0   CoreFoundation                      0x00007fff3855fd07 __exceptionPreprocess + 250
1   libobjc.A.dylib                     0x00007fff712845bf objc_exception_throw + 48
2   CoreFoundation                      0x00007fff385dec97 -[NSObject(NSObject) __retain_OA] + 0
3   CoreFoundation                      0x00007fff384c457b ___forwarding___ + 1427
4   CoreFoundation                      0x00007fff3858aa88 __forwarding_prep_1___ + 120
5   AppKit                              0x00007fff357f2f11 -[NSScrollerImp _unsafeRectForPart:] + 1500
6   AppKit                              0x00007fff357f31cf -[NSOverlayScrollerImp _unsafeRectForPart:] + 147
7   AppKit                              0x00007fff357f28de -[NSScrollerImp _threadsafeRectForPart:preBlock:postBlock:] + 82
8   AppKit                              0x00007fff359cd206 -[NSOverlayScrollerImp expandedRectForPart:] + 154
9   AppKit                              0x00007fff359cd0d7 -[NSScrollerImp _rolloverTrackingRect] + 45
10  AppKit                              0x00007fff359d5ffc -[NSScrollerImpPair _beginHideOverlayScrollers] + 296
11  Foundation                          0x00007fff3abd53ca __NSFireTimer + 67
12  CoreFoundation                      0x00007fff384fe9b9 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
13  CoreFoundation                      0x00007fff384fe51f __CFRunLoopDoTimer + 859
14  CoreFoundation                      0x00007fff384fe007 __CFRunLoopDoTimers + 322
15  CoreFoundation                      0x00007fff384e2daa __CFRunLoopRun + 1871
16  CoreFoundation                      0x00007fff384e1ffe CFRunLoopRunSpecific + 462
17  DumpRenderTree                      0x0000000106288e84 _ZL7runTestRKNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEEE + 2675
18  DumpRenderTree                      0x0000000106288025 _Z14dumpRenderTreeiPPKc + 2933
19  DumpRenderTree                      0x0000000106289a8c _Z18DumpRenderTreeMainiPPKc + 1490
20  libdyld.dylib                       0x00007fff7242bcc9 start + 1
21  ???                                 0x0000000000000002 0x0 + 2

[Ryan Haddad]

Maybe related to:

Make it possible to test overlay scrollbar interactions
https://trac.webkit.org/changeset/261056/webkit

[David Kilzer (:ddkilzer)]

(In reply to Truitt Savell from comment #0)
> Crash Log:
> https://build.webkit.org/results/Apple-Catalina-Release-WK1-Tests/
> r261078%20(5479)/inspector/console/console-api-crash-log.txt

This crash looks like an over-released Objective-C object (that was later replaced by another object of a different type):


Application Specific Information:
CRASHING TEST: inspector/console/console-api.html
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[__NSMallocBlock__ convertRectToBacking:]: unrecognized selector sent to instance 0x7fefcf5f0a70'
abort() called
terminating with uncaught exception of type NSException


This might have caught the bug sooner:
Bug 211245: EWS bots should run with NSZombieEnabled=YES

Also, this is a crash in AppKit scrolling code.

[Simon Fraser (smfr)]

Oddly none of the new tests were run in the same shard before the crash.

[Simon Fraser (smfr)]

Oh but I also made overlay scrollbars actually be overlay scrollbars. That must be it.

Note this is WK1.

[Radar WebKit Bug Importer]

<rdar://problem/62851084>

[Truitt Savell]

It looks like two other tests are also crashing related to this:
inspector/console/command-line-api-getEventListeners.html
inspector/console/command-line-api.html

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&test=inspector%2Fconsole%2Fcommand-line-api-getEventListeners.html&test=inspector%2Fconsole%2Fcommand-line-api.html&test=inspector%2Fconsole%2Fconsole-api.html

[Simon Fraser (smfr)]

inspector/console/command-line-api-copy.html is the only test that triggers overlay scrollbars

[Simon Fraser (smfr)]

Seems like createPageForSanitizingWebContent() doesn't get the memo that DRT uses mock scrollbars.

[Simon Fraser (smfr)]

I can't reproduce crashes on Catalina, nor can I detect NSScrollerImp left with bad delegates.

[Simon Fraser (smfr)]

Created attachment 398397 [details]
Patch

[Simon Fraser (smfr)]

Landed the logging in https://trac.webkit.org/changeset/261102/webkit, let's see what happens.

[Simon Fraser (smfr)]

Looks like we do have crashes from Catalina too:
https://build.webkit.org/builders/Apple-Catalina-Debug-WK1-Tests/builds/4319

[Simon Fraser (smfr)]

Logging didn't reveal anything.

[Simon Fraser (smfr)]

Can't reproduce with run-webkit-tests inspector/console/command-line-api-copy.html inspector/console/command-line-api-getEventListeners.html -1 --iterations=100 -f -g

[Ryan Haddad]

(In reply to Simon Fraser (smfr) from comment #14)
> Logging didn't reveal anything.
The logging upset the Perf bot: https://build.webkit.org/builders/Apple-Catalina-Release-WK2-Perf/builds/851

Can we go ahead and revert it?

[Simon Fraser (smfr)]

Yes: bug 211418.

[Simon Fraser (smfr)]

I can reproduce. Logging shows that we lost the NSScrollerImp before clearing the delegate:

worker/0 inspector/console/command-line-api-copy.html output stderr lines:

 ScrollAnimatorMac 0x122608700 didAddVerticalScrollbar 0x1226cad48: setting delegate 0x7f9255d6ac60 on imp 0x7f9255d56510
 ScrollAnimatorMac 0x122608700 willRemoveVerticalScrollbar 0x1226cad48: clearing delegate 0x0 on imp 0x7f9255d56510
...
 ScrollAnimatorMac 0x122608e00 didAddVerticalScrollbar 0x1226ca898: setting delegate 0x7f9255d3a000 on imp 0x7f9255d65760
 ScrollAnimatorMac 0x122608e00 willRemoveVerticalScrollbar 0x1226ca898: clearing delegate 0x0 on imp 0x0

[Simon Fraser (smfr)]

We’re clearing the ScrollbarThemeMac which holds the mapping between Scrollbars and NSScrollerImps, so we lost the ability to find the NSScrollerImp for a given scrollbar, and leave the delegate dangling. So this is definitely a test-only crash.

[Simon Fraser (smfr)]

`-[WebInspectorWindowController init] is enabling overlay scrollbars when running inspector/console/command-line-api-copy.html, then at the end of the test we re-enable mock scrollbars via gTestRunner->setDeveloperExtrasEnabled(false) then load about:blank, and at that point no longer disassociate the Scrollbar from its NSScrollerImp.

[Simon Fraser (smfr)]

Created attachment 398661 [details]
Patch

[Tim Horton]

Comment on attachment 398661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398661&action=review

> Source/WebCore/ChangeLog:8
> +        This bug was caused by the failure clear the delegate on an NSScrollerImp when, for testing,

There is at least one missing or extra word here

> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:185
> +        [iter->value.get() setDelegate:nil];

.get() here? I'm not sure what `value` is , but hopefully unnecessary

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/261256/webkit

[Darin Adler]

Comment on attachment 398661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398661&action=review

> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:187
> -    scrollbarMap()->remove(&scrollbar);
> +    auto iter = scrollbarMap().find(&scrollbar);
> +    if (iter != scrollbarMap().end()) {
> +        [iter->value.get() setDelegate:nil];
> +        scrollbarMap().remove(iter);
> +    }

This can be written really nicely using "take()". I think this will work:

    [scrollbarMap()->take(&scrollbar) setDelegate:nil];

Because Objective-C messages quietly do nothing with nil, don’t even need an if statement.

[Darin Adler]

Comment on attachment 398661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398661&action=review

>> Source/WebCore/platform/mac/ScrollbarThemeMac.mm:187
>> +    }
> 
> This can be written really nicely using "take()". I think this will work:
> 
>     [scrollbarMap()->take(&scrollbar) setDelegate:nil];
> 
> Because Objective-C messages quietly do nothing with nil, don’t even need an if statement.

[scrollbarMap().take(&scrollbar) setDelegate:nil];

[Simon Fraser (smfr)]

Did that in https://trac.webkit.org/changeset/261306/webkit