Bug 213391 – com.apple.WebKit.Networking crash: suspended with locked system files (observations.db)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

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.

[Kate Cheney]

<rdar://problem/64494167>

[Kate Cheney]

Created attachment 402307 [details]
Patch

[Kate Cheney]

Created attachment 402325 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

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

[Darin Adler]

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

[Kate Cheney]

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.

[Kate Cheney]

Created attachment 402330 [details]
Patch

[Chris Dumez]

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?

[Kate Cheney]

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.

[Chris Dumez]

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

[Kate Cheney]

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

[Kate Cheney]

Created attachment 402334 [details]
Patch for landing

[Kate Cheney]

Created attachment 402344 [details]
Patch for landing

[Kate Cheney]

postTask ASSERTS to check for Ephemeral sessions, I added an early return for the ephemeral case.

[EWS]

Committed r263298: <https://trac.webkit.org/changeset/263298>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402344 [details].