It seems that closing the network process checks for locked system files. NetworkProcess::didClose() kicks off closing of the ITP database, which locks the observations.db file and causes the crash.
<rdar://problem/64494167>
Created attachment 402307 [details] Patch
Created attachment 402325 [details] Patch
Comment on attachment 402307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402307&action=review > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:231 > + flushAndDestroyPersistentStore([callbackAggregator = callbackAggregator.copyRef()] { }); > + destroyResourceLoadStatisticsStore([callbackAggregator = callbackAggregator.copyRef()] { }); copyRef() no longer is necessary, so it can be written like this now, with the same effect: flushAndDestroyPersistentStore([callbackAggregator] { }); destroyResourceLoadStatisticsStore([callbackAggregator] { }); > Source/WebKit/NetworkProcess/NetworkProcess.cpp:270 > + networkSession.flushAndDestroyPersistentStore([callbackAggregator = callbackAggregator.copyRef()] { }); Same here, don’t need copyRef() any more.
(In reply to Darin Adler from comment #4) > Comment on attachment 402307 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=402307&action=review > > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:231 > > + flushAndDestroyPersistentStore([callbackAggregator = callbackAggregator.copyRef()] { }); > > + destroyResourceLoadStatisticsStore([callbackAggregator = callbackAggregator.copyRef()] { }); > > copyRef() no longer is necessary, so it can be written like this now, with > the same effect: > > flushAndDestroyPersistentStore([callbackAggregator] { }); > destroyResourceLoadStatisticsStore([callbackAggregator] { }); > > > Source/WebKit/NetworkProcess/NetworkProcess.cpp:270 > > + networkSession.flushAndDestroyPersistentStore([callbackAggregator = callbackAggregator.copyRef()] { }); > > Same here, don’t need copyRef() any more. Any reason we did not drop copyRef() on Ref now that it is copyable?
(In reply to Chris Dumez from comment #5) > Any reason we did not drop copyRef() on Ref now that it is copyable? Just haven’t gotten to it yet. It’s marked "deprecated". Someone just has to remove all the calls to copyRef then we can delete it.
Comment on attachment 402307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402307&action=review >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:270 >> + networkSession.flushAndDestroyPersistentStore([callbackAggregator = callbackAggregator.copyRef()] { }); > > Same here, don’t need copyRef() any more. Noted! I'll change this for both instances.
Created attachment 402330 [details] Patch
Comment on attachment 402325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402325&action=review > Source/WebKit/ChangeLog:10 > + NetworkProcess::didClose() is called. This causes the network process When does NetworkProcess::didClose() get called? Isn't it when the UIProcess process has quit?
Comment on attachment 402325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402325&action=review >> Source/WebKit/ChangeLog:10 >> + NetworkProcess::didClose() is called. This causes the network process > > When does NetworkProcess::didClose() get called? Isn't it when the UIProcess process has quit? Yes, I believe so. Local storage DB does not close when this happens, I don't think ITP should either if the network session hasn't been destroyed.
Comment on attachment 402330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402330&action=review > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:255 > + m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] () mutable { postTask() ? > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:257 > + RunLoop::main().dispatch(WTFMove(completionHandler)); postTaskReply() ?
Comment on attachment 402330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=402330&action=review >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:255 >> + m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] () mutable { > > postTask() ? Will change! >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:257 >> + RunLoop::main().dispatch(WTFMove(completionHandler)); > > postTaskReply() ? Ditto
Created attachment 402334 [details] Patch for landing
Created attachment 402344 [details] Patch for landing
postTask ASSERTS to check for Ephemeral sessions, I added an early return for the ephemeral case.
Committed r263298: <https://trac.webkit.org/changeset/263298> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402344 [details].