Bug 175721 - REGRESSION (r220601): Crash when closing google doc after switching the order of tabs in safari
Summary: REGRESSION (r220601): Crash when closing google doc after switching the order...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 212537
  Show dependency treegraph
 
Reported: 2017-08-18 09:20 PDT by Sam Weinig
Modified: 2020-05-29 11:45 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.46 KB, patch)
2017-08-18 12:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2017-08-18 09:20:15 PDT
https://trac.webkit.org/changeset/220601 caused the following crash on iOS.

Thread 0 Crashed:
0   WebKit                        	0x00000001005ece90 WTF::Function<void ()>::CallableWrapper<WebKit::WebProcess::markAllLayersVolatile(WTF::Function<void ()>&&)::$_7>::call() + 28
1   WebKit                        	0x00000001005576f0 WebKit::WebPage::callVolatilityCompletionHandlers() + 72
2   WebKit                        	0x00000001005578f8 WebKit::WebPage::markLayersVolatile(WTF::Function<void ()>&&) + 488
3   WebKit                        	0x00000001005e8f78 WebKit::WebProcess::markAllLayersVolatile(WTF::Function<void ()>&&) + 292
4   WebKit                        	0x00000001005e8d78 WebKit::WebProcess::actualPrepareToSuspend(WebKit::WebProcess::ShouldAcknowledgeWhenReadyToSuspend) + 112
5   WebKit                        	0x00000001005e91a4 WebKit::WebProcess::prepareToSuspend() + 160
6   WebKit                        	0x00000001003da8b0 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 164
7   WebKit                        	0x00000001003dd2a4 IPC::Connection::dispatchOneMessage() + 232
8   JavaScriptCore                	0x0000000101e16bb0 WTF::RunLoop::performWork() + 196
9   JavaScriptCore                	0x0000000101e16dcc WTF::RunLoop::performWork(void*) + 36
10  CoreFoundation                	0x00000001858a4358 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CF/CF-1443/RunLoop.subproj/CFRunLoop.c:1982)
11  CoreFoundation                	0x00000001858a42d8 __CFRunLoopDoSource0 + 88 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CF/CF-1443/RunLoop.subproj/CFRunLoop.c:2017)
12  CoreFoundation                	0x00000001858a3b60 __CFRunLoopDoSources0 + 204 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CF/CF-1443/RunLoop.subproj/CFRunLoop.c:2053)
13  CoreFoundation                	0x00000001858a1738 __CFRunLoopRun + 1048 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CF/CF-1443/RunLoop.subproj/CFRunLoop.c:2920)
14  CoreFoundation                	0x00000001857c22d8 CFRunLoopRunSpecific + 436 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CF/CF-1443/RunLoop.subproj/CFRunLoop.c:3245)
15  Foundation                    	0x00000001861ea6e4 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 304 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/Foundation/Foundation-1444.12/Soil.subproj/NSRunLoop.m:367)
16  Foundation                    	0x000000018623c62c -[NSRunLoop(NSRunLoop) run] + 88 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/Foundation/Foundation-1444.12/Soil.subproj/NSRunLoop.m:389)
17  libxpc.dylib                  	0x00000001855702b0 _xpc_objc_main + 516 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libxpc/libxpc-1205.20.17/src/main.m:167)
18  libxpc.dylib                  	0x000000018557233c xpc_main + 180 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libxpc/libxpc-1205.20.17/src/init.c:1470)
19  com.apple.WebKit.WebContent   	0x0000000100187594 0x100184000 + 13716
20  libdyld.dylib                 	0x00000001852e656c start + 4

It is being tracked in radar with <rdar://problem/33928369>.

The likely culprit is WebProcess::markAllLayersVolatile() which is moving a WTF::Function multiple times (for each page, see lines 1392-1393) leading nulled out functions being called. It looks like this was previously broken, but silent due to to the old nature of WTF::Function silently ignoring calls to null functions.
Comment 1 Chris Dumez 2017-08-18 11:01:11 PDT
<rdar://problem/33928369>
Comment 2 Chris Dumez 2017-08-18 12:29:46 PDT
Created attachment 318527 [details]
Patch
Comment 3 Geoffrey Garen 2017-08-18 12:41:54 PDT
Comment on attachment 318527 [details]
Patch

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

> Source/WebKit/WebProcess/WebProcess.cpp:1388
> +    m_pageMarkingLayersAsVolatileCounter = std::make_unique<PageMarkingLayersAsVolatileCounter>([this, completionHandler = WTFMove(completionHandler)] (RefCounterEvent) {

How do we know that the WebProcess object will still be alive when our callback fires?
Comment 4 Geoffrey Garen 2017-08-18 12:43:19 PDT
Comment on attachment 318527 [details]
Patch

r=me

Still a little sketched out by the refcounting here.
Comment 5 Chris Dumez 2017-08-18 12:47:28 PDT
Comment on attachment 318527 [details]
Patch

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

>> Source/WebKit/WebProcess/WebProcess.cpp:1388
>> +    m_pageMarkingLayersAsVolatileCounter = std::make_unique<PageMarkingLayersAsVolatileCounter>([this, completionHandler = WTFMove(completionHandler)] (RefCounterEvent) {
> 
> How do we know that the WebProcess object will still be alive when our callback fires?

If the WebProcess does away then so does the RefCounter object, since it is a data member. If the RefCounter object is destroyed, the token gets disconnected from the RefCounter object and the valueDidChange callback cannot get called.
Comment 6 Geoffrey Garen 2017-08-18 12:51:03 PDT
> If the WebProcess does away then so does the RefCounter object, since it is
> a data member.

Oh, duh. :P
Comment 7 WebKit Commit Bot 2017-08-18 13:53:36 PDT
Comment on attachment 318527 [details]
Patch

Clearing flags on attachment: 318527

Committed r220931: <http://trac.webkit.org/changeset/220931>
Comment 8 WebKit Commit Bot 2017-08-18 13:53:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Sam Weinig 2017-08-18 14:10:45 PDT
Heh. I guess I should have assigned this to myself when I filed it if I didn't want you to fix it before I did :). Thanks for tackling it!
Comment 10 Chris Dumez 2017-08-18 14:27:05 PDT
(In reply to Sam Weinig from comment #9)
> Heh. I guess I should have assigned this to myself when I filed it if I
> didn't want you to fix it before I did :). Thanks for tackling it!

Oh yes :) I saw you did not assign it to yourself and cc'd me and took it a a sign you wanted me to take it if I had time.
Comment 11 Daniel Bates 2020-05-29 11:23:56 PDT
Comment on attachment 318527 [details]
Patch

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

> Source/WebKit/WebProcess/WebProcess.cpp:1390
> +            return;

Completion handler never called.
Comment 12 Chris Dumez 2020-05-29 11:45:04 PDT
Comment on attachment 318527 [details]
Patch

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

>> Source/WebKit/WebProcess/WebProcess.cpp:1390
>> +            return;
> 
> Completion handler never called.

Well, the idea is that it is supposed to get called below when m_pageMarkingLayersAsVolatileCounter->value() becomes 0.