More work on LocalStorageDatabaseTracker

• Component: New Bugs
• Status: RESOLVED FIXED

[Anders Carlsson]

More work on LocalStorageDatabaseTracker

[Anders Carlsson]

Created attachment 200851 [details]
Patch

[Andreas Kling]

Comment on attachment 200851 [details]
Patch

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

r=me

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:96
> +    if (!fileExists(databasePath) && openingStrategy == SkipIfNonExistent)
> +        return;
> +
> +    if (!m_database.open(databasePath)) {
> +        LOG_ERROR("Failed to open databasePath %s.", databasePath.ascii().data());
> +        return;
> +    }

There's a race between checking if the file exists and opening it. Looks a bit shoddy.

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:98
> +    m_database.disableThreadingChecks();

This needs a comment.

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:111
> +    openTrackerDatabase(SkipIfNonExistent);
> +
> +    if (m_database.isOpen()) {

I'd make openTrackerDatabase() return a bool for success and branch on that instead.

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:129
> +    updateTrackerDatabaseFromLocalStorageDatabaseFiles();

Should we still be doing this if opening the database failed?

[Anders Carlsson]

(In reply to comment #2)
> (From update of attachment 200851 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200851&action=review
> 
> r=me
> 
> > Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:96
> > +    if (!fileExists(databasePath) && openingStrategy == SkipIfNonExistent)
> > +        return;
> > +
> > +    if (!m_database.open(databasePath)) {
> > +        LOG_ERROR("Failed to open databasePath %s.", databasePath.ascii().data());
> > +        return;
> > +    }
> 
> There's a race between checking if the file exists and opening it. Looks a bit shoddy.

OK. What can happen is that the database is deleted between the call to fileExists and in that case it’ll be re-created by the call to Database::open.

> 
> > Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:129
> > +    updateTrackerDatabaseFromLocalStorageDatabaseFiles();
> 
> Should we still be doing this if opening the database failed?

It’ll still update the oh-so-important in-process data structures.

[Anders Carlsson]

Committed r149647: <http://trac.webkit.org/changeset/149647>

[Ryosuke Niwa]

It appears that this patch broke WK2 bots:
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK2%20%28Tests%29?numbuilds=50

e.g.
16:33:53.229 46880 worker/1 accessibility/accessibility-node-memory-management.html crashed, (stderr lines):
16:33:53.229 46880   ASSERTION FAILED: !m_localStorageDirectory
16:33:53.229 46880   /Volumes/Data/slave/mountainlion-debug/build/Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp(65) : void WebKit::LocalStorageDatabaseTracker::setLocalStorageDirectoryInternal(const WTF::String &)
16:33:53.229 46880   1   0x10b985093 WebKit::LocalStorageDatabaseTracker::setLocalStorageDirectoryInternal(WTF::String const&)
16:33:53.229 46880   2   0x10b98639a WTF::FunctionWrapper<void (WebKit::LocalStorageDatabaseTracker::*)(WTF::String const&)>::operator()(WebKit::LocalStorageDatabaseTracker*, WTF::String const&)
16:33:53.229 46880   3   0x10b98630c WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebKit::LocalStorageDatabaseTracker::*)(WTF::String const&)>, void (WebKit::LocalStorageDatabaseTracker*, WTF::String)>::operator()()
16:33:53.229 46880   4   0x10b6e3939 WTF::Function<void ()>::operator()() const
16:33:53.229 46880   5   0x10bb18be1 __dispatch_block_invoke_0
16:33:53.229 46880   6   0x7fff8c62bf01 _dispatch_call_block_and_release
16:33:53.229 46880   7   0x7fff8c6280b6 _dispatch_client_callout
16:33:53.229 46880   8   0x7fff8c62947f _dispatch_queue_drain
16:33:53.229 46880   9   0x7fff8c6292f1 _dispatch_queue_invoke
16:33:53.229 46880   10  0x7fff8c6291c3 _dispatch_worker_thread2
16:33:53.229 46880   11  0x7fff8d7f2d0b _pthread_wqthread
16:33:53.229 46880   12  0x7fff8d7dd1d1 start_wqthread

I can reproduce this crash locally.

[Ryosuke Niwa]

Filed https://bugs.webkit.org/show_bug.cgi?id=115711 to track the assertion failure.