We should abort ITP database transactions that are not finished when the network process suspends, otherwise the unfinished transaction will hold the lock of the database file.
<rdar://problem/60540768>
Created attachment 400755 [details] Patch
I talked with Sihui offline and we are not convinced aborting transactions will solve the issue, mainly because there is time between when NetworkProcess::prepareToSuspend() is called and when the process actually suspends. So it is unlikely that a previous transaction in the ITP database is still occurring after the process has suspended. This indicates the problem could be that the thread is not being stopped properly in the call to WebResourceLoadStatistics::suspend(). I posted a patch adding debug asserts to this area of code in: https://bugs.webkit.org/show_bug.cgi?id=212663 to get more information.
(In reply to katherine_cheney from comment #3) > I talked with Sihui offline and we are not convinced aborting transactions > will solve the issue, mainly because there is time between when > NetworkProcess::prepareToSuspend() is called and when the process actually > suspends. So it is unlikely that a previous transaction in the ITP database > is still occurring after the process has suspended. This indicates the > problem could be that the thread is not being stopped properly in the call > to WebResourceLoadStatistics::suspend(). > > I posted a patch adding debug asserts to this area of code in: > https://bugs.webkit.org/show_bug.cgi?id=212663 to get more information. If aborting transactions is what IDB does, I think ITP should do the same. A transaction can take a large amount of time when when you get a PrepareToSuspend, you may get suspended fairly quick after, especially in the case of imminent suspension.
Comment on attachment 400755 [details] Patch I am 90% sure this is wrong and re-introducing a bug that we fixed recently. I am a bit sad that the API tests are not failing since I think we added a test to cover that. The reason WebResourceLoadStatisticsStore was static was that there is only one shared queue nowadays. You don't want to go through every store and hang each queue separately because they all share the same queue..
(In reply to Chris Dumez from comment #5) > Comment on attachment 400755 [details] > Patch > > I am 90% sure this is wrong and re-introducing a bug that we fixed recently. > I am a bit sad that the API tests are not failing since I think we added a > test to cover that. The reason WebResourceLoadStatisticsStore was static was > that there is only one shared queue nowadays. You don't want to go through > every store and hang each queue separately because they all share the same > queue.. See Bug 211207.
(In reply to Chris Dumez from comment #5) > Comment on attachment 400755 [details] > Patch > > I am 90% sure this is wrong and re-introducing a bug that we fixed recently. > I am a bit sad that the API tests are not failing since I think we added a > test to cover that. The reason WebResourceLoadStatisticsStore was static was > that there is only one shared queue nowadays. You don't want to go through > every store and hang each queue separately because they all share the same > queue.. Hmm I see. I am not sure how to access the database object from a static function, then, am I missing something obvious?
(In reply to katherine_cheney from comment #7) > (In reply to Chris Dumez from comment #5) > > Comment on attachment 400755 [details] > > Patch > > > > I am 90% sure this is wrong and re-introducing a bug that we fixed recently. > > I am a bit sad that the API tests are not failing since I think we added a > > test to cover that. The reason WebResourceLoadStatisticsStore was static was > > that there is only one shared queue nowadays. You don't want to go through > > every store and hang each queue separately because they all share the same > > queue.. > > Hmm I see. I am not sure how to access the database object from a static > function, then, am I missing something obvious? Maybe you can keep a static HashMap of all ResourceLoadStatisticsDatabaseStore objects which you only modify / query from the background ITP queue. Then in the static suspend, you dispatch to the background queue, iterate through each ResourceLoadStatisticsDatabaseStore object in the map to interrupt their transactions and only then suspend the thread.
Created attachment 400873 [details] Patch
Comment on attachment 400873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400873&action=review Please add a static allStores() function to ResourceLoadStatisticsDatabaseStore and move the logic there. Then you add to the HashSet in the ResourceLoadStatisticsDatabaseStore constructor and remove in the ResourceLoadStatisticsDatabaseStore destructor. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:178 > + static NeverDestroyed<HashMap<WebResourceLoadStatisticsStore*, ResourceLoadStatisticsDatabaseStore*>> map; Why isn't this a HashSet<ResourceLoadStatisticsDatabaseStore*> ? > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:179 > + return map; I would ASSERT(!RunLoop::isMain()); in here. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:199 > + statisticsDatabaseStoreMap().add(this, downcast<ResourceLoadStatisticsDatabaseStore>(m_statisticsStore.get())); You add but never remove.
Comment on attachment 400873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400873&action=review > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1436 > + for (auto& databaseStore : statisticsDatabaseStoreMap().values()) Your code is not thread-safe either since you iterate over the map in the background thread but populate the map in the main thread. When I suggested this, I said the HashMap should only be modified / queried on the background queue.
Comment on attachment 400873 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400873&action=review >> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1436 >> + for (auto& databaseStore : statisticsDatabaseStoreMap().values()) > > Your code is not thread-safe either since you iterate over the map in the background thread but populate the map in the main thread. When I suggested this, I said the HashMap should only be modified / queried on the background queue. Never mind this comment, from the diff output, I missed that you were populating in the background queue.
Created attachment 400946 [details] Patch
Comment on attachment 400946 [details] Patch r=me
Committed r262505: <https://trac.webkit.org/changeset/262505> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400946 [details].