| Summary: | GPU process occasionally crashes under `StreamConnectionWorkQueue::processStreams()` in layout tests | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Wenson Hsieh <wenson_hsieh> | ||||||
| Component: | WebKit2 | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cdumez, kkinnunen, simon.fraser, thorton, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=231939 https://bugs.webkit.org/show_bug.cgi?id=231940 |
||||||||
| Attachments: |
|
||||||||
|
Description
Wenson Hsieh
2021-10-18 08:58:48 PDT
Created attachment 441611 [details]
Crash log
I can intermittently reproduce by running the tests in `compositing` on 4 iOS 15 simulators in parallel for ~3-4 iterations. Created attachment 441650 [details]
Fixes the tests
Comment on attachment 441650 [details]
Fixes the tests
Thanks for the review!
Committed r284427 (243191@main): <https://commits.webkit.org/243191@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 441650 [details]. Comment on attachment 441650 [details] Fixes the tests View in context: https://bugs.webkit.org/attachment.cgi?id=441650&action=review > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:102 > + m_workQueue->stop(); Is there a reason to do the ref-related changes? Wasn't the stop the only thing that was missing? I don't think the ref made above lives longer than the stop. (In reply to Kimmo Kinnunen from comment #7) > Comment on attachment 441650 [details] > Fixes the tests > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441650&action=review > > > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:102 > > + m_workQueue->stop(); > > Is there a reason to do the ref-related changes? > Wasn't the stop the only thing that was missing? > I don't think the ref made above lives longer than the stop. It's true that this call to `stop()` was technically the only change required to avoid the crash. However, given that StreamConnectionWorkQueue is ref-counted, it was also wrong to be storing `m_workQueue` directly as a member on RemoteRenderingBackend, rather than through some kind of Ref-counted template class (i.e. Ref or RefPtr). This would be important if (for example) we were to make `stop()` non-blocking in the future. |