Bug 165478 - In iOS, we should take background assertion when accessing localstorage databases.
Summary: In iOS, we should take background assertion when accessing localstorage datab...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yongjun Zhang
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-12-06 10:45 PST by Yongjun Zhang
Modified: 2017-02-03 09:52 PST (History)
14 users (show)

See Also:


Attachments
Patch. (31.83 KB, patch)
2016-12-06 12:08 PST, Yongjun Zhang
no flags Details | Formatted Diff | Diff
Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it. (43.71 KB, patch)
2017-01-14 20:35 PST, Yongjun Zhang
no flags Details | Formatted Diff | Diff
Fix iOS build break (1.88 KB, patch)
2017-01-15 14:27 PST, Yongjun Zhang
no flags Details | Formatted Diff | Diff
Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it. (43.71 KB, patch)
2017-01-15 14:30 PST, Yongjun Zhang
beidson: review+
Details | Formatted Diff | Diff
Address review comments before landing. (43.80 KB, patch)
2017-02-01 21:31 PST, Yongjun Zhang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yongjun Zhang 2016-12-06 10:45:40 PST
In WK2, we access localstorage databases in UIProcess. We should take the assertion to keep UIProcess active when database is being accessed.
Comment 1 Yongjun Zhang 2016-12-06 10:46:10 PST
<rdar://problem/26685576>
Comment 2 Yongjun Zhang 2016-12-06 12:08:07 PST
Created attachment 296304 [details]
Patch.
Comment 3 Chris Dumez 2016-12-06 12:13:34 PST
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 ?
Comment 4 Chris Dumez 2016-12-06 12:14:42 PST
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/?
Comment 5 Yongjun Zhang 2016-12-06 12:19:44 PST
(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.
Comment 6 Chris Dumez 2016-12-06 12:25:16 PST
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?
Comment 7 Yongjun Zhang 2016-12-06 12:30:09 PST
(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.
Comment 8 Darin Adler 2016-12-20 15:42:51 PST
Seems like Gavin or Brady should review, if no one else is planning to.
Comment 9 Brady Eidson 2017-01-04 12:48:35 PST
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.
Comment 10 Yongjun Zhang 2017-01-07 14:59:18 PST
(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.
Comment 11 Yongjun Zhang 2017-01-14 20:35:03 PST
Created attachment 298881 [details]
Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it.
Comment 12 Yongjun Zhang 2017-01-15 14:27:08 PST
Created attachment 298931 [details]
Fix iOS build break
Comment 13 Yongjun Zhang 2017-01-15 14:30:26 PST
Created attachment 298932 [details]
Move database transaction background task handling to WebCore so that both WK1 and WK2 can use it.
Comment 14 Brady Eidson 2017-01-20 09:23:48 PST
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.
Comment 15 Yongjun Zhang 2017-01-26 21:34:54 PST
(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.
Comment 16 Brady Eidson 2017-02-01 13:25:45 PST
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
Comment 17 Yongjun Zhang 2017-02-01 21:31:08 PST
Created attachment 300388 [details]
Address review comments before landing.
Comment 18 WebKit Commit Bot 2017-02-02 00:34:55 PST
Comment on attachment 300388 [details]
Address review comments before landing.

Clearing flags on attachment: 300388

Committed r211551: <http://trac.webkit.org/changeset/211551>
Comment 19 WebKit Commit Bot 2017-02-02 00:35:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Csaba Osztrogonác 2017-02-02 14:06:58 PST
(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
Comment 21 Csaba Osztrogonác 2017-02-03 04:51:27 PST
fixes landed in https://trac.webkit.org/changeset/211626 
and https://trac.webkit.org/changeset/211628
Comment 22 Yongjun Zhang 2017-02-03 09:52:33 PST
(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!