Bug 186584 – Crash under SWServer::unregisterConnection(Connection&)

• Component: Service Workers
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

<rdar://problem/40931680>

[Chris Dumez]

Created attachment 342625 [details]
Patch

[Chris Dumez]

Created attachment 342626 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Darin Adler]

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.

[Chris Dumez]

Created attachment 342658 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

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.

[youenn fablet]

Comment on attachment 342658 [details]
Patch

LGTM.
A question though: would it simplify the overall model if we were making SWServer owning its connections?

[Chris Dumez]

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

[Chris Dumez]

Created attachment 342703 [details]
Alternative (SWServer owns the connections)

[Chris Dumez]

Created attachment 342704 [details]
Alternative (SWServer owns the connections)

[Chris Dumez]

Let me know which one you prefer. Both patches are about the same size.

[youenn fablet]

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?

[Chris Dumez]

(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+ ?

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 342716 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 342716 [details]
Patch

Clearing flags on attachment: 342716

Committed r232824: <https://trac.webkit.org/changeset/232824>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.