Bug 186584

Summary: Crash under SWServer::unregisterConnection(Connection&)
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Service WorkersAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, commit-queue, darin, ews, ggaren, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
ews: commit-queue-
Archive of layout-test-results from ews113 for mac-sierra
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews206 for win-future
none
Alternative (SWServer owns the connections)
none
Alternative (SWServer owns the connections)
none
Archive of layout-test-results from ews106 for mac-sierra-wk2
none
Patch none

Description Chris Dumez 2018-06-12 19:26:52 PDT
Crash under SWServer::unregisterConnection(Connection&):
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010a63bc60 WTFCrash + 16 (Assertions.cpp:267)
1   com.apple.WebCore             	0x000000011562690a WebCore::SWServer::unregisterConnection(WebCore::SWServer::Connection&) + 154 (SWServer.cpp:704)
2   com.apple.WebCore             	0x0000000115626842 WebCore::SWServer::Connection::~Connection() + 50 (SWServer.cpp:63)
3   com.apple.WebKit              	0x00000001046e809b WebKit::WebSWServerConnection::~WebSWServerConnection() + 411 (WebSWServerConnection.cpp:77)
4   com.apple.WebKit              	0x00000001046e82d5 WebKit::WebSWServerConnection::~WebSWServerConnection() + 21 (WebSWServerConnection.cpp:77)
5   com.apple.WebKit              	0x00000001046e8339 WebKit::WebSWServerConnection::~WebSWServerConnection() + 25 (WebSWServerConnection.cpp:73)
6   com.apple.WebKit              	0x0000000103ee57f5 WTF::VectorDestructor<true, std::__1::unique_ptr<WebKit::WebSWServerConnection, std::__1::default_delete<WebKit::WebSWServerConnection> > >::destruct(std::__1::unique_ptr<WebKit::WebSWServerConnection, std::__1::default_delete<WebKit::WebSWServerConnection> >*, std::__1::unique_ptr<WebKit::WebSWServerConnection, std::__1::default_delete<WebKit::WebSWServerConnection> >*) + 213 (memory:2598)
7   com.apple.WebKit              	0x0000000103ee56ad WTF::VectorTypeOperations<std::__1::unique_ptr<WebKit::WebSWServerConnection, std::__1::default_delete<WebKit::WebSWServerConnection> > >::destruct(std::__1::unique_ptr<WebKit::WebSWServerConnection, std::__1::default_delete<WebKit::WebSWServerConnection> >*, std::__1::unique_ptr<WebKit::WebSWServerConnection, std::__1::default_delete<WebKit::WebSWServerConnection> >*) + 29 (Vector.h:241)
8   com.apple.WebKit              	0x0000000103ee5670 WTF::Vector<std::__1::unique_ptr<WebKit::WebSWServerConnection, std::__1::default_delete<WebKit::WebSWServerConnection> >, 0ul, WTF::CrashOnOverflow, 16ul>::~Vector() + 64
9   com.apple.WebKit              	0x0000000103ee1405 WTF::Vector<std::__1::unique_ptr<WebKit::WebSWServerConnection, std::__1::default_delete<WebKit::WebSWServerConnection> >, 0ul, WTF::CrashOnOverflow, 16ul>::~Vector() + 21 (Vector.h:670)
10  com.apple.WebKit              	0x0000000103ee0ca2 WebKit::StorageToWebProcessConnection::didClose(IPC::Connection&) + 978 (StorageToWebProcessConnection.cpp:156)
11  com.apple.WebKit              	0x00000001038f8966 IPC::Connection::connectionDidClose()::$_13::operator()() + 246 (Connection.cpp:811)
12  com.apple.WebKit              	0x00000001038f87e9 WTF::Function<void ()>::CallableWrapper<IPC::Connection::connectionDidClose()::$_13>::call() + 25 (Function.h:101)
13  com.apple.JavaScriptCore      	0x000000010a66060d WTF::Function<void ()>::operator()() const + 141 (Function.h:56)
14  com.apple.JavaScriptCore      	0x000000010a6b4333 WTF::RunLoop::performWork() + 211 (RunLoop.cpp:107)
15  com.apple.JavaScriptCore      	0x000000010a6b4c44 WTF::RunLoop::performWork(void*) + 36 (RunLoopCF.cpp:38)
16  com.apple.CoreFoundation      	0x00007fff37ca4470 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 1
Comment 1 Chris Dumez 2018-06-12 19:27:12 PDT
<rdar://problem/40931680>
Comment 2 Chris Dumez 2018-06-12 20:56:15 PDT
Created attachment 342625 [details]
Patch
Comment 3 Chris Dumez 2018-06-12 21:07:51 PDT
Created attachment 342626 [details]
Patch
Comment 4 Build Bot 2018-06-12 22:45:18 PDT
Comment on attachment 342626 [details]
Patch

Attachment 342626 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/8156468

New failing tests:
js/dom/JSON-stringify.html
Comment 5 Build Bot 2018-06-12 22:45:20 PDT
Created attachment 342633 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 6 Build Bot 2018-06-12 23:00:26 PDT
Comment on attachment 342626 [details]
Patch

Attachment 342626 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/8156675

New failing tests:
http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-with-partitioning-timeout.html
Comment 7 Build Bot 2018-06-12 23:00:27 PDT
Created attachment 342636 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 8 Darin Adler 2018-06-12 23:32:25 PDT
Comment on attachment 342626 [details]
Patch

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

> Source/WebCore/workers/service/server/SWServer.cpp:62
> +    server().unregisterConnection(*this);

Can’t m_server be null here?

> Source/WebCore/workers/service/server/SWServer.cpp:79
> +    while (!m_connections.isEmpty())
> +        (*m_connections.values().begin())->connectionInvalidated();

Why does calling SWServer::Connection::connectionInvalidated guarantee that the connection will be removed from m_connections? It seems like there is a missing call to unregisterConnection.
Comment 9 Chris Dumez 2018-06-13 08:37:34 PDT
Created attachment 342658 [details]
Patch
Comment 10 Build Bot 2018-06-13 12:47:23 PDT
Comment on attachment 342658 [details]
Patch

Attachment 342658 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8165947

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
Comment 11 Build Bot 2018-06-13 12:47:34 PDT
Created attachment 342685 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 12 Chris Dumez 2018-06-13 14:32:06 PDT
Comment on attachment 342685 [details]
Archive of layout-test-results from ews206 for win-future

Windows failure cannot be related as this change is WK2 only.
Comment 13 youenn fablet 2018-06-13 14:48:45 PDT
Comment on attachment 342658 [details]
Patch

LGTM.
A question though: would it simplify the overall model if we were making SWServer owning its connections?
Comment 14 Chris Dumez 2018-06-13 15:02:20 PDT
(In reply to youenn fablet from comment #13)
> Comment on attachment 342658 [details]
> Patch
> 
> LGTM.
> A question though: would it simplify the overall model if we were making
> SWServer owning its connections?

I have this other patch too, I implemented both. I'll upload the other one so you can compare.
Comment 15 Chris Dumez 2018-06-13 16:08:40 PDT
Created attachment 342703 [details]
Alternative (SWServer owns the connections)
Comment 16 Chris Dumez 2018-06-13 16:11:25 PDT
Created attachment 342704 [details]
Alternative (SWServer owns the connections)
Comment 17 Chris Dumez 2018-06-13 16:38:20 PDT
Let me know which one you prefer. Both patches are about the same size.
Comment 18 youenn fablet 2018-06-13 16:54:37 PDT
Comment on attachment 342704 [details]
Alternative (SWServer owns the connections)

(In reply to Chris Dumez from comment #17)
> Let me know which one you prefer. Both patches are about the same size.

I slightly prefer the second patch, it seems simpler to understand in terms of who owns who.

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

> Source/WebCore/workers/service/server/SWServer.h:150
> +    Connection* connection(SWServerConnectionIdentifier identifier) { return m_connections.get(identifier); }

const?
Comment 19 Chris Dumez 2018-06-13 16:56:20 PDT
(In reply to youenn fablet from comment #18)
> Comment on attachment 342704 [details]
> Alternative (SWServer owns the connections)
> 
> (In reply to Chris Dumez from comment #17)
> > Let me know which one you prefer. Both patches are about the same size.
> 
> I slightly prefer the second patch, it seems simpler to understand in terms
> of who owns who.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342704&action=review
> 
> > Source/WebCore/workers/service/server/SWServer.h:150
> > +    Connection* connection(SWServerConnectionIdentifier identifier) { return m_connections.get(identifier); }
> 
> const?

Ok, r+ ?
Comment 20 Build Bot 2018-06-13 17:24:19 PDT
Comment on attachment 342704 [details]
Alternative (SWServer owns the connections)

Attachment 342704 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/8170319

New failing tests:
imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.html
Comment 21 Build Bot 2018-06-13 17:24:20 PDT
Created attachment 342711 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 22 Chris Dumez 2018-06-13 18:18:47 PDT
Created attachment 342716 [details]
Patch
Comment 23 WebKit Commit Bot 2018-06-13 19:31:36 PDT
Comment on attachment 342716 [details]
Patch

Clearing flags on attachment: 342716

Committed r232824: <https://trac.webkit.org/changeset/232824>
Comment 24 WebKit Commit Bot 2018-06-13 19:31:38 PDT
All reviewed patches have been landed.  Closing bug.