<rdar://problem/41325767>
Created attachment 375064 [details] Patch
Comment on attachment 375064 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375064&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:624 > + swap(m_networkProcess, networkProcess); Indentation problem? Also, why swap instead of a WTFMove()? If you're right about the reason for the crash, wouldn't it mean that ensureNetworkProcess() is now likely to re-enter and re-construct m_networkProcess?
Created attachment 375092 [details] Patch
Comment on attachment 375092 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375092&action=review > Source/WebKit/UIProcess/WebProcessPool.cpp:475 > + if (!RunLoop::isMain()) { Can we at lease add a ASSERT(RunLoop::isMain()); debug assertion? > Source/WebKit/UIProcess/WebProcessPool.cpp:476 > + callOnMainRunLoopAndWait([this] { I am not convinced |this| is guaranteed to still be alive by the time you get to the main runloop. We likely this to protect/ref it in the lambda.
(In reply to Chris Dumez from comment #4) > Comment on attachment 375092 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375092&action=review > > > Source/WebKit/UIProcess/WebProcessPool.cpp:475 > > + if (!RunLoop::isMain()) { > > Can we at lease add a ASSERT(RunLoop::isMain()); debug assertion? Okay. > > > Source/WebKit/UIProcess/WebProcessPool.cpp:476 > > + callOnMainRunLoopAndWait([this] { > > I am not convinced |this| is guaranteed to still be alive by the time you > get to the main runloop. We likely this to protect/ref it in the lambda. Will add.
Created attachment 375093 [details] Patch
Comment on attachment 375093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375093&action=review r=me > Source/WebKit/UIProcess/WebProcessPool.cpp:476 > + if (!RunLoop::isMain()) { We may want to put in a comment explaining this is a temporary workaround for apps using our API on non-main threads.
Comment on attachment 375093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375093&action=review >> Source/WebKit/UIProcess/WebProcessPool.cpp:476 >> + if (!RunLoop::isMain()) { > > We may want to put in a comment explaining this is a temporary workaround for apps using our API on non-main threads. Yes, let's please do that, with a link to a bug report. That way, future programmers have context, and will know under what conditions they can remove this / what they should test if they remove this.
(In reply to Geoffrey Garen from comment #8) > Comment on attachment 375093 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375093&action=review > > >> Source/WebKit/UIProcess/WebProcessPool.cpp:476 > >> + if (!RunLoop::isMain()) { > > > > We may want to put in a comment explaining this is a temporary workaround for apps using our API on non-main threads. > > Yes, let's please do that, with a link to a bug report. That way, future > programmers have context, and will know under what conditions they can > remove this / what they should test if they remove this. Sure, will add after the last bot turns green.
Created attachment 375127 [details] Patch for landing
Comment on attachment 375127 [details] Patch for landing Clearing flags on attachment: 375127 Committed r247933: <https://trac.webkit.org/changeset/247933>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53687367>