Bug 144661 – [iOS] Close all open databases in expiration handler of process assertion

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

[Daniel Bates]

We should interrupt all in-progress database transactions on expiration of a UIProcess process assertion so that the WebProcess is not terminated on suspension because it is holding a lock on a database file (since it is performing a database transaction).

[Daniel Bates]

Created attachment 252444 [details]
Part 1: Add infrastructure to interrupt all databases

[Radar WebKit Bug Importer]

<rdar://problem/20845052>

[Daniel Bates]

Created attachment 252681 [details]
Patch

[Daniel Bates]

Created attachment 252682 [details]
Patch

Remove whitespace inadvertently added to -[WebDatabaseManager endBackgroundTask].

[Darin Adler]

Comment on attachment 252682 [details]
Patch

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

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:311
> +    Vector<RefPtr<Database>> openDatabases;

Might be able to use Vector<Ref<Database>> here instead.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:321
> +        for (auto& databaseOrigin : *m_openDatabaseMap) {
> +            for (auto& databaseName : *databaseOrigin.value) {
> +                for (auto& database : *databaseName.value)
> +                    openDatabases.append(database);
> +            }
> +        }

This should be written using values():

    for (auto& nameMap : m_openDatabaseMap->values()) {
        for (auto& set : nameMap->values()) {
            for (auto& database : *set)
                openDatabases.append(database);
        }
    }

> Source/WebKit2/UIProcess/ProcessThrottler.h:44
> +class ProcessThrottler : public ProcessAssertionClient {

Please make this private inheritance instead.

> Source/WebKit2/UIProcess/ProcessThrottler.h:61
> +    // ProcessAssertionClient
> +    void assertionWillExpireImminently() override;

Please make this private.

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:47
> +- (void)addClient:(ProcessAssertionClient *)client;
> +- (void)removeClient:(ProcessAssertionClient *)client;

Wrong formatting. ProcessAssertionClient isn’t an Objective-C class. Also, these arguments should be references, not pointers.

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:56
> +    HashSet<ProcessAssertionClient *> _clients;

Wrong formatting. ProcessAssertionClient isn’t an Objective-C class.

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:111
> +            for (auto* client : _clients)
> +                client->assertionWillExpireImminently();

What guarantees the body of assertionWillExpireImminently won’t call something that could reenter and call addClient: or removeClient: while iterating _clients?

[Daniel Bates]

(In reply to comment #5)
> > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:311
> > +    Vector<RefPtr<Database>> openDatabases;
> 
> Might be able to use Vector<Ref<Database>> here instead.
> 

Will use Vector<Ref<Database>>.

> > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:321
> > +        for (auto& databaseOrigin : *m_openDatabaseMap) {
> > +            for (auto& databaseName : *databaseOrigin.value) {
> > +                for (auto& database : *databaseName.value)
> > +                    openDatabases.append(database);
> > +            }
> > +        }
> 
> This should be written using values():
> 
>     for (auto& nameMap : m_openDatabaseMap->values()) {
>         for (auto& set : nameMap->values()) {
>             for (auto& database : *set)
>                 openDatabases.append(database);
>         }
>     }
> 

Will fix.

> > Source/WebKit2/UIProcess/ProcessThrottler.h:44
> > +class ProcessThrottler : public ProcessAssertionClient {
> 
> Please make this private inheritance instead.
> 

Will use private inheritance.

> > Source/WebKit2/UIProcess/ProcessThrottler.h:61
> > +    // ProcessAssertionClient
> > +    void assertionWillExpireImminently() override;
> 
> Please make this private.
> 

Will make this private.

> > Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:47
> > +- (void)addClient:(ProcessAssertionClient *)client;
> > +- (void)removeClient:(ProcessAssertionClient *)client;
> 
> Wrong formatting. ProcessAssertionClient isn’t an Objective-C class. Also,
> these arguments should be references, not pointers.
> 

Will use references instead of pointers and fix formatting.

> > Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:56
> > +    HashSet<ProcessAssertionClient *> _clients;
> 
> Wrong formatting. ProcessAssertionClient isn’t an Objective-C class.
> 

Will fix formatting.

> > Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:111
> > +            for (auto* client : _clients)
> > +                client->assertionWillExpireImminently();
> 
> What guarantees the body of assertionWillExpireImminently won’t call
> something that could reenter and call addClient: or removeClient: while
> iterating _clients?

Will copy HashSet<ProcessAssertionClient*> to a vector and iterate over the vector to prevent issues with such reentrancy.

[Daniel Bates]

Committed r184105: <http://trac.webkit.org/changeset/184105>

[Daniel Bates]

(In reply to comment #6)
> > > Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:111
> > > +            for (auto* client : _clients)
> > > +                client->assertionWillExpireImminently();
> > 
> > What guarantees the body of assertionWillExpireImminently won’t call
> > something that could reenter and call addClient: or removeClient: while
> > iterating _clients?
> 
> Will copy HashSet<ProcessAssertionClient*> to a vector and iterate over the
> vector to prevent issues with such reentrancy.

We should also ensure that client is still in the list of clients managed by WKProcessAssertionBackgroundTaskManager. Filed bug #144880 to address this.