Bug 212608 – Any active sqlite transactions for the ITP database should be aborted when the network process suspends.

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

[Kate Cheney]

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.

[Kate Cheney]

<rdar://problem/60540768>

[Kate Cheney]

Created attachment 400755 [details]
Patch

[Kate Cheney]

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.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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

[Kate Cheney]

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

[Chris Dumez]

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

[Kate Cheney]

Created attachment 400873 [details]
Patch

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Kate Cheney]

Created attachment 400946 [details]
Patch

[Chris Dumez]

Comment on attachment 400946 [details]
Patch

r=me

[EWS]

Committed r262505: <https://trac.webkit.org/changeset/262505>

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