Bug 165478 – In iOS, we should take background assertion when accessing localstorage databases.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Yongjun Zhang]

In WK2, we access localstorage databases in UIProcess. We should take the assertion to keep UIProcess active when database is being accessed.

[Yongjun Zhang]

<rdar://problem/26685576>

[Yongjun Zhang]

Created attachment 296304 [details]
Patch.

[Chris Dumez]

Comment on attachment 296304 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=296304&action=review

> Source/WebKit2/UIProcess/ios/WebSQLiteDatabaseTrackerClient.h:32
> +class WebSQLiteDatabaseTrackerClient : public WebCore::SQLiteDatabaseTrackerClient {

What about the pre-existing Source/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.h ?

[Chris Dumez]

Comment on attachment 296304 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=296304&action=review

>> Source/WebKit2/UIProcess/ios/WebSQLiteDatabaseTrackerClient.h:32
>> +class WebSQLiteDatabaseTrackerClient : public WebCore::SQLiteDatabaseTrackerClient {
> 
> What about the pre-existing Source/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.h ?

What I mean is: Is there a way to merge the 2 in WebCore under platform/?

[Yongjun Zhang]

(In reply to comment #3)
> Comment on attachment 296304 [details]
> Patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296304&action=review
> 
> > Source/WebKit2/UIProcess/ios/WebSQLiteDatabaseTrackerClient.h:32
> > +class WebSQLiteDatabaseTrackerClient : public WebCore::SQLiteDatabaseTrackerClient {
> 
> What about the pre-existing
> Source/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.h ?

Under the hood, the original WebSQLiteDatabaseTrackerClient uses WK1 WebDatabaseManager, which is not available in WK2. Also, I think it would be more efficient to use the same background assertion in ProcessAssertion class, instead of creating another task assertion.

[Chris Dumez]

Comment on attachment 296304 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=296304&action=review

>>>> Source/WebKit2/UIProcess/ios/WebSQLiteDatabaseTrackerClient.h:32
>>>> +class WebSQLiteDatabaseTrackerClient : public WebCore::SQLiteDatabaseTrackerClient {
>>> 
>>> What about the pre-existing Source/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.h ?
>> 
>> What I mean is: Is there a way to merge the 2 in WebCore under platform/?
> 
> Under the hood, the original WebSQLiteDatabaseTrackerClient uses WK1 WebDatabaseManager, which is not available in WK2. Also, I think it would be more efficient to use the same background assertion in ProcessAssertion class, instead of creating another task assertion.

What about the WebSQLiteDatabaseTracker that is already in WebKit2?

[Yongjun Zhang]

(In reply to comment #6)
> Comment on attachment 296304 [details]
> Patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296304&action=review
> 
> >>>> Source/WebKit2/UIProcess/ios/WebSQLiteDatabaseTrackerClient.h:32
> >>>> +class WebSQLiteDatabaseTrackerClient : public WebCore::SQLiteDatabaseTrackerClient {
> >>> 
> >>> What about the pre-existing Source/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.h ?
> >> 
> >> What I mean is: Is there a way to merge the 2 in WebCore under platform/?
> > 
> > Under the hood, the original WebSQLiteDatabaseTrackerClient uses WK1 WebDatabaseManager, which is not available in WK2. Also, I think it would be more efficient to use the same background assertion in ProcessAssertion class, instead of creating another task assertion.
> 
> What about the WebSQLiteDatabaseTracker that is already in WebKit2?

This is only used in child processes like Network process and WebProcess, but local storage databases are handled in UIProcess.

[Darin Adler]

Seems like Gavin or Brady should review, if no one else is planning to.

[Brady Eidson]

Comment on attachment 296304 [details]
Patch.

I think I'm with Chris on this; We should find a way to move both the WK1 and WK2 implementations of this to WebCore.

Then SQLiteTransaction can take the assertion itself, which would always solve this problem everywhere once and for all.

[Yongjun Zhang]

(In reply to comment #9)
> Comment on attachment 296304 [details]
> Patch.
> 
> I think I'm with Chris on this; We should find a way to move both the WK1
> and WK2 implementations of this to WebCore.
> 
> Then SQLiteTransaction can take the assertion itself, which would always
> solve this problem everywhere once and for all.

I agree, this would be ideal. However, moving the background task assertion into WebCore means we need to indirectly add UIKit dependencies for WebCore since the assertion is in UIApplication. I will look at possible ways to achieve this without introducing UIKit dependencies.

[Yongjun Zhang]

Created attachment 298881 [details]
Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it.

[Yongjun Zhang]

Created attachment 298931 [details]
Fix iOS build break

[Yongjun Zhang]

Created attachment 298932 [details]
Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it.

[Brady Eidson]

Comment on attachment 298932 [details]
Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it.

This isn't quite what I had in mind.

The concepts of "an in-progress SQLite transaction" and "an open SQLite database" are both WebCore concepts, and yet this patch still seems to leave tons of logic in the WebKits.

I expected to see much more code down in WebCore as well as much less difference between WK1 and WK2.

I'm not sure this is better.

[Yongjun Zhang]

(In reply to comment #14)
> Comment on attachment 298932 [details]
> Move database transaction background task handling to WebCore so that both
> WK1 and WK2 can use it.
> 
> This isn't quite what I had in mind.
> 
> The concepts of "an in-progress SQLite transaction" and "an open SQLite
> database" are both WebCore concepts, and yet this patch still seems to leave
> tons of logic in the WebKits.
> 
> I expected to see much more code down in WebCore as well as much less
> difference between WK1 and WK2.
> 
> I'm not sure this is better.

As discussed in person, this patch moves the background tasks handling into WebCore, which is one step closer to our ultimate goal - keep "SQLite transaction" all inside WebCore. Since WebCore can't have dependency on UIKit, we still need to do the minimum setup in WK1 and WK2 to pass the background task handlers to WebCore, which is essentially what this patch does.

[Brady Eidson]

Comment on attachment 298932 [details]
Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it.

View in context: https://bugs.webkit.org/attachment.cgi?id=298932&action=review

> Source/WebCore/platform/ios/WebBackgroundTaskController.hSource/WebKit/ios/Storage/WebSQLiteDatabaseTrackerClient.mm:2
> - * Copyright (C) 2010 Apple Inc. All Rights Reserved.
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

2010, 2017

> Source/WebCore/platform/ios/WebSQLiteDatabaseTrackerClient.mm:2
> + * Copyright (C) 2010 Apple Inc. All Rights Reserved.

2017

[Yongjun Zhang]

Created attachment 300388 [details]
Address review comments before landing.

[WebKit Commit Bot]

Comment on attachment 300388 [details]
Address review comments before landing.

Clearing flags on attachment: 300388

Committed r211551: <http://trac.webkit.org/changeset/211551>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #18)
> Comment on attachment 300388 [details]
> Address review comments before landing.
> 
> Clearing flags on attachment: 300388
> 
> Committed r211551: <http://trac.webkit.org/changeset/211551>

It broke the Apple Mac cmake build, see this for details:
https://build.webkit.org/builders/Apple%20El%20Capitan%20CMake%20Debug%20%28Build%29/builds/11745

[Csaba Osztrogonác]

fixes landed in https://trac.webkit.org/changeset/211626 
and https://trac.webkit.org/changeset/211628

[Yongjun Zhang]

(In reply to comment #21)
> fixes landed in https://trac.webkit.org/changeset/211626 
> and https://trac.webkit.org/changeset/211628

Thank you for fixing this!