Bug 211386 - REGRESSION (r261056): [ Mac WK1 ] inspector/console/console-api.html is flaky crashing
Summary: REGRESSION (r261056): [ Mac WK1 ] inspector/console/console-api.html is flaky...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on: 211418
Blocks:
  Show dependency treegraph
 
Reported: 2020-05-04 09:23 PDT by Truitt Savell
Modified: 2020-05-07 11:10 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.66 KB, patch)
2020-05-04 12:08 PDT, Simon Fraser (smfr)
ddkilzer: review+
Details | Formatted Diff | Diff
Patch (10.73 KB, patch)
2020-05-06 14:46 PDT, Simon Fraser (smfr)
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Ryan Haddad 2020-05-04 10:15:24 PDT
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
Comment 2 Ryan Haddad 2020-05-04 10:15:59 PDT
Maybe related to:

Make it possible to test overlay scrollbar interactions
https://trac.webkit.org/changeset/261056/webkit
Comment 3 David Kilzer (:ddkilzer) 2020-05-04 10:17:13 PDT
(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.
Comment 4 Simon Fraser (smfr) 2020-05-04 10:26:37 PDT
Oddly none of the new tests were run in the same shard before the crash.
Comment 5 Simon Fraser (smfr) 2020-05-04 10:27:05 PDT
Oh but I also made overlay scrollbars actually be overlay scrollbars. That must be it.

Note this is WK1.
Comment 6 Radar WebKit Bug Importer 2020-05-04 10:30:46 PDT
<rdar://problem/62851084>
Comment 7 Truitt Savell 2020-05-04 10:56:47 PDT
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
Comment 8 Simon Fraser (smfr) 2020-05-04 11:54:07 PDT
inspector/console/command-line-api-copy.html is the only test that triggers overlay scrollbars
Comment 9 Simon Fraser (smfr) 2020-05-04 11:56:40 PDT
Seems like createPageForSanitizingWebContent() doesn't get the memo that DRT uses mock scrollbars.
Comment 10 Simon Fraser (smfr) 2020-05-04 12:00:07 PDT
I can't reproduce crashes on Catalina, nor can I detect NSScrollerImp left with bad delegates.
Comment 11 Simon Fraser (smfr) 2020-05-04 12:08:28 PDT
Created attachment 398397 [details]
Patch
Comment 12 Simon Fraser (smfr) 2020-05-04 12:53:03 PDT
Landed the logging in https://trac.webkit.org/changeset/261102/webkit, let's see what happens.
Comment 13 Simon Fraser (smfr) 2020-05-04 15:27:47 PDT
Looks like we do have crashes from Catalina too:
https://build.webkit.org/builders/Apple-Catalina-Debug-WK1-Tests/builds/4319
Comment 14 Simon Fraser (smfr) 2020-05-04 15:31:20 PDT
Logging didn't reveal anything.
Comment 15 Simon Fraser (smfr) 2020-05-04 15:38:05 PDT
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
Comment 16 Ryan Haddad 2020-05-04 17:03:30 PDT
(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?
Comment 17 Simon Fraser (smfr) 2020-05-04 17:36:03 PDT
Yes: bug 211418.
Comment 18 Simon Fraser (smfr) 2020-05-06 14:17:34 PDT
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
Comment 19 Simon Fraser (smfr) 2020-05-06 14:17:40 PDT
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.
Comment 20 Simon Fraser (smfr) 2020-05-06 14:22:24 PDT
`-[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.
Comment 21 Simon Fraser (smfr) 2020-05-06 14:46:38 PDT
Created attachment 398661 [details]
Patch
Comment 22 Tim Horton 2020-05-06 14:50:11 PDT
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
Comment 23 Simon Fraser (smfr) 2020-05-06 15:56:33 PDT
https://trac.webkit.org/changeset/261256/webkit
Comment 24 Darin Adler 2020-05-06 15:59:33 PDT
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.
Comment 25 Darin Adler 2020-05-06 16:00:22 PDT
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];
Comment 26 Simon Fraser (smfr) 2020-05-07 11:10:42 PDT
Did that in https://trac.webkit.org/changeset/261306/webkit