| Summary: | Crash under SWServer::unregisterConnection(Connection&) | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||||
| Component: | Service Workers | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | beidson, commit-queue, darin, ews-watchlist, ggaren, rniwa, webkit-bug-importer, youennf | ||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Chris Dumez
2018-06-12 19:26:52 PDT
Created attachment 342625 [details]
Patch
Created attachment 342626 [details]
Patch
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 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 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 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 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. Created attachment 342658 [details]
Patch
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 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 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 on attachment 342658 [details]
Patch
LGTM.
A question though: would it simplify the overall model if we were making SWServer owning its connections?
(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. Created attachment 342703 [details]
Alternative (SWServer owns the connections)
Created attachment 342704 [details]
Alternative (SWServer owns the connections)
Let me know which one you prefer. Both patches are about the same size. 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? (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 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 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
Created attachment 342716 [details]
Patch
Comment on attachment 342716 [details] Patch Clearing flags on attachment: 342716 Committed r232824: <https://trac.webkit.org/changeset/232824> All reviewed patches have been landed. Closing bug. |