Bug 135346 – Make WKOriginDataManager actually operate on IndexedDatabases

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

[Brady Eidson]

Make WKOriginDataManager correctly operate on IndexedDatabases

This patch will get the list of origins with databases by checking for their entries in the directory structure on disk.
It will also delete the database files (and any resulting empty directories) when requested.

<rdar://problem/15917314>

[Brady Eidson]

In this patch, open handles to the database files on disk will not be notified and invalidated.

This is probably quite an acceptable situation for now. because:
    - For read-only operations, they will continue functioning as normal on the unlinked file.
    - For write operations, they will start producing errors as SQLite notices the missing backing store.

But it should still be fixed, and I filed https://bugs.webkit.org/show_bug.cgi?id=135347 to track that in a followup

[Brady Eidson]

Created attachment 235611 [details]
Patch v1

[Sam Weinig]

Comment on attachment 235611 [details]
Patch v1

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

> Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:60
> +    // FIXME (<rdar://problem/9127819>): Is there a more explicit way of doing this besides accessing the UTF8Encoding?

This should probably be a bugzilla bug.

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:116
> +    if (!callback) {
> +        LOG_ERROR("WebOriginDataManagerProxy::didGetOrigins - Invalid callback ID");
> +        return;
> +    }
> +

When can this happen?  Should this be a MESSAGE_CHECK()?

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:131
> +    if (!(types & kWKIndexedDatabaseData)) {
> +        callbackFunction(CallbackBase::Error::None);
>          return;
> +    }

This should probably have the same comment as above (or just move the one from below).

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:160
> +    if (!(types & kWKIndexedDatabaseData)) {
> +        callbackFunction(CallbackBase::Error::None);
>          return;
> +    }

This should probably have the same comment as above (or just move the one from below).

> Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:190
> +    if (!(types & kWKIndexedDatabaseData)) {
> +        callbackFunction(CallbackBase::Error::None);
>          return;
> +    }

This should probably have the same comment as above (or just move the one from below).

[Brady Eidson]

(In reply to comment #3)
> (From update of attachment 235611 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235611&action=review
> 
> > Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp:60
> > +    // FIXME (<rdar://problem/9127819>): Is there a more explicit way of doing this besides accessing the UTF8Encoding?
> 
> This should probably be a bugzilla bug.

Yah, probably!

> > Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:116
> > +    if (!callback) {
> > +        LOG_ERROR("WebOriginDataManagerProxy::didGetOrigins - Invalid callback ID");
> > +        return;
> > +    }
> > +
> 
> When can this happen?  Should this be a MESSAGE_CHECK()?

Shouldn't happen.  Should probably be a MESSAGE_CHECK.

> This should probably have the same comment as above (or just move the one from below).
...
> This should probably have the same comment as above (or just move the one from below).
...
> This should probably have the same comment as above (or just move the one from below).

Willdo

[Brady Eidson]

Filed https://bugs.webkit.org/show_bug.cgi?id=135365 for that radar.  Will update the comment in the patch (as well as the comment in LocalStorageDatabaseTracker.cpp) before landing.

[Brady Eidson]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > > Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:116
> > > +    if (!callback) {
> > > +        LOG_ERROR("WebOriginDataManagerProxy::didGetOrigins - Invalid callback ID");
> > > +        return;
> > > +    }
> > > +
> > 
> > When can this happen?  Should this be a MESSAGE_CHECK()?
> 
> Shouldn't happen.  Should probably be a MESSAGE_CHECK.

MESSAGE_CHECK requires you know what Connection the current dispatched message came from.

Right now, that can only be the DatabaseProcess, so I can do it.

In the future, WebOriginDataManagerProxy will *not* know what Connection this message is coming from, so we'll have to deal with that later.

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > 
> > > > Source/WebKit2/UIProcess/WebOriginDataManagerProxy.cpp:116
> > > > +    if (!callback) {
> > > > +        LOG_ERROR("WebOriginDataManagerProxy::didGetOrigins - Invalid callback ID");
> > > > +        return;
> > > > +    }
> > > > +
> > > 
> > > When can this happen?  Should this be a MESSAGE_CHECK()?
> > 
> > Shouldn't happen.  Should probably be a MESSAGE_CHECK.
> 
> MESSAGE_CHECK requires you know what Connection the current dispatched message came from.
> 
> Right now, that can only be the DatabaseProcess, so I can do it.
> 
> In the future, WebOriginDataManagerProxy will *not* know what Connection this message is coming from, so we'll have to deal with that later.

Actually with the magic of WantsConnection, we can do this.

New patch forthcoming.

[Brady Eidson]

Created attachment 235647 [details]
Patch v2 - Also added a new API we'll need very soon anyways

[Brady Eidson]

Sorry about the "Also added a new API we'll need very soon anyways" - That got in there from autocomplete history.

This V2 is basically V1 with review feedback addressed.

[David Kilzer (:ddkilzer)]

Comment on attachment 235647 [details]
Patch v2 - Also added a new API we'll need very soon anyways

r=me on the parts of the patch that touch the filesystem.

[Alex Christensen]

(In reply to comment #10)
> (From update of attachment 235647 [details])
> r=me on the parts of the patch that touch the filesystem.
me, too!

[Brady Eidson]

http://trac.webkit.org/changeset/171749

[Csaba Osztrogonác]

(In reply to comment #12)
> http://trac.webkit.org/changeset/171749

and the buildfix landed in http://trac.webkit.org/changeset/171753