Bug 88338 – IndexedDB: should make the LevelDB persistant to the directory indicated in PageGroupSettings::indexedDBDataBasePath

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Charles Wei]

Seems now the LevelDB for IndexedDB is created in memory, 

IDBFactory::open()  
{
   ...
   m_backend->open(name, request.get(), context->securityOrigin(), frame, String());
   ...
}

The last parameter gives an empty string, which will cause the LevelDB creates an in-memory database instead of file-system database.  Shouldn't it be located in Page::GroupSettings::indexedDataBasePath() ? if you do want to create an in-memory database,  you can set Page::GroupSettings::indexedDataBasePath() to empty.

[Joshua Bell]

The Chromium port overrides the path between this call and when IDBFactoryBackendImpl sees it.

Having this default to Page::GroupSettings::indexedDataBasePath() (and allowing ports to override) should be fine, except that IIRC GroupSettings is not available to Workers so some additional plumbing will be needed.

[Charles Wei]

(In reply to comment #1)
> The Chromium port overrides the path between this call and when IDBFactoryBackendImpl sees it.
> 
> Having this default to Page::GroupSettings::indexedDataBasePath() (and allowing ports to override) should be fine, except that IIRC GroupSettings is not available to Workers so some additional plumbing will be needed.

Joshua: will you please point to me the Chromium port that overrides the LevelDB database path ? I didn't find it.

(In reply to comment #2)
> (In reply to comment #1)
> > The Chromium port overrides the path between this call and when IDBFactoryBackendImpl sees it.
> > 
> > Having this default to Page::GroupSettings::indexedDataBasePath() (and allowing ports to override) should be fine, except that IIRC GroupSettings is not available to Workers so some additional plumbing will be needed.
> 
> Joshua: will you please point to me the Chromium port that overrides the LevelDB database path ? I didn't find it.

In IndexedDBDispatcherHost::OnIDBFactoryOpen in http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/in_process_webkit/indexed_db_dispatcher_host.cc?view=markup

[Charles Wei]

(In reply to comment #1)
> The Chromium port overrides the path between this call and when IDBFactoryBackendImpl sees it.
> 
> Having this default to Page::GroupSettings::indexedDataBasePath() (and allowing ports to override) should be fine, except that IIRC GroupSettings is not available to Workers so some additional plumbing will be needed.

You are right, there's no access to GroupSettings in WorkerContext.  Actually the Worker object itself should have access to that.  The problem is now how to get the Worker object from the WorkerContext ? Is that the right direction ?

[Charles Wei]

Created attachment 146528 [details]
Patch

What about just stashing the path into the WorkerContext when the WorkerContext is created?

[Joshua Bell]

(In reply to comment #6)
> What about just stashing the path into the WorkerContext when the WorkerContext is created?

Or possibly give each WorkerContext a copy of the source page's GroupSettings object? (I don't know this part of the code very well.)

...

As an aside, to enable IDB with Workers for a non-Chromium port, https://bugs.webkit.org/show_bug.cgi?id=82776 will probably have to be tackled. It's critical that all pages/workers in an origin share a factory, as multiple leveldb instances can't open the same path simultaneously (unlike e.g. sqlite)

[Charles Wei]

Anybody can help to review ?

(In reply to comment #8)
> Anybody can help to review ?

You haven't yet adressed the old comments?

[Charles Wei]

Created attachment 148693 [details]
Patch

[WebKit Review Bot]

Comment on attachment 148693 [details]
Patch

Attachment 148693 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13010112

[Charles Wei]

Created attachment 148716 [details]
Patch

[WebKit Review Bot]

Comment on attachment 148716 [details]
Patch

Attachment 148716 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13020072

[Charles Wei]

Created attachment 148725 [details]
Patch

[WebKit Review Bot]

Comment on attachment 148725 [details]
Patch

Attachment 148725 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13028025

[Charles Wei]

Created attachment 148735 [details]
Patch

[WebKit Review Bot]

Comment on attachment 148735 [details]
Patch

Attachment 148735 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13039015

[Charles Wei]

Created attachment 148756 [details]
Patch

Comment on attachment 148756 [details]
Patch

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

> Source/WebCore/workers/WorkerContext.h:70
> +        const GroupSettings* groupSettings() { return m_thread->groupSettings(); }

any reason not to use workerContext->thread()->groupSettings() everywhere?

[Charles Wei]

Comment on attachment 148756 [details]
Patch

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

>> Source/WebCore/workers/WorkerContext.h:70
>> +        const GroupSettings* groupSettings() { return m_thread->groupSettings(); }
> 
> any reason not to use workerContext->thread()->groupSettings() everywhere?

Well, that definitely works. But providing this API to WorkerContext saves the client code (workContext->groupSettings() instead of workerContext->thread()->groupSettings()),  and gives the developer direct access to settings via WorkerContext instead of through WorkerThread. 

Jochen:  do you have any concerns about this?

(In reply to comment #20)
> (From update of attachment 148756 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148756&action=review
> 
> >> Source/WebCore/workers/WorkerContext.h:70
> >> +        const GroupSettings* groupSettings() { return m_thread->groupSettings(); }
> > 
> > any reason not to use workerContext->thread()->groupSettings() everywhere?
> 
> Well, that definitely works. But providing this API to WorkerContext saves the client code (workContext->groupSettings() instead of workerContext->thread()->groupSettings()),  and gives the developer direct access to settings via WorkerContext instead of through WorkerThread. 
> 
> Jochen:  do you have any concerns about this?

It strikes me as unusual to have such a short-cut method when the object is accessible anyway

[Charles Wei]

Created attachment 149239 [details]
Patch

[Charles Wei]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 148756 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=148756&action=review
> > 
> > >> Source/WebCore/workers/WorkerContext.h:70
> > >> +        const GroupSettings* groupSettings() { return m_thread->groupSettings(); }
> > > 
> > > any reason not to use workerContext->thread()->groupSettings() everywhere?
> > 
> > Well, that definitely works. But providing this API to WorkerContext saves the client code (workContext->groupSettings() instead of workerContext->thread()->groupSettings()),  and gives the developer direct access to settings via WorkerContext instead of through WorkerThread. 
> > 
> > Jochen:  do you have any concerns about this?
> 
> It strikes me as unusual to have such a short-cut method when the object is accessible anyway

Ok, the latest patch addresses this.

Comment on attachment 149239 [details]
Patch

The latest patch sets is missing the changed to IDB?

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

> Source/WebCore/workers/DefaultSharedWorkerRepository.cpp:100
> +    GroupSettings* groupSettings() const; // Page GroupSettings used by worker thread.

nit. add empty line before this one

[Charles Wei]

(In reply to comment #24)
> (From update of attachment 149239 [details])
> The latest patch sets is missing the changed to IDB?
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=149239&action=review
> 
> > Source/WebCore/workers/DefaultSharedWorkerRepository.cpp:100
> > +    GroupSettings* groupSettings() const; // Page GroupSettings used by worker thread.
> 
> nit. add empty line before this one

Thanks, Jochen. Anyone has any other comments ? I can make the change to address jochen's comments when submitting the patch.

Seems I was blind and didn't see the IDBFactory change, sorry

Anyway, there seems to be a potential NULL ptr reference, as the WorkerThread might be invoked with a NULL GroupSettings ptr. Can you check that the group settings exist before dereffing it?

[Charles Wei]

Created attachment 149685 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149685 [details]
Patch

Attachment 149685 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13103673

[Charles Wei]

Created attachment 149706 [details]
Patch

(In reply to comment #29)
> Created an attachment (id=149706) [details]
> Patch

This change looks good

[Charles Wei]

(In reply to comment #30)
> (In reply to comment #29)
> > Created an attachment (id=149706) [details] [details]
> > Patch
> 
> This change looks good

Thanks, Jochen. I need a r+ to proceed landing the patch :-)

[WebKit Review Bot]

Comment on attachment 149706 [details]
Patch

Attachment 149706 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13103721

New failing tests:
platform/chromium/compositing/layout-width-change.html
platform/chromium/compositing/render-surface-alpha-blending.html
platform/chromium/compositing/tiny-layer-rotated.html
platform/chromium/compositing/huge-layer-rotated.html
fast/loader/loadInProgress.html
platform/chromium/compositing/3d-corners.html
platform/chromium/compositing/video-frame-size-change.html
platform/chromium/compositing/perpendicular-layer-sorting.html
platform/chromium/compositing/plugins/webplugin-no-alpha.html
platform/chromium/compositing/child-layer-3d-sorting.html
platform/chromium/compositing/lost-compositor-context-permanently.html
platform/chromium/compositing/filters/background-filter-blur-outsets.html
platform/chromium/compositing/lost-compositor-context-with-video.html
platform/chromium/compositing/accelerated-drawing/alpha.html
http/tests/xmlhttprequest/zero-length-response.html
http/tests/security/script-crossorigin-loads-correctly.html
platform/chromium/compositing/plugins/webplugin-alpha.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/backface-visibility-transformed.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html
fast/loader/unload-form-post-about-blank.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/img-layer-grow.html
platform/chromium/compositing/filters/background-filter-blur-off-axis.html
platform/chromium/compositing/filters/background-filter-blur.html
platform/chromium/compositing/lost-compositor-context-twice.html

[WebKit Review Bot]

Created attachment 149713 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Charles Wei]

Comment on attachment 149706 [details]
Patch

I really don't see how it causes regressions for chromium. Let my just re-set the cq flag to give it a try.

[Charles Wei]

Created attachment 149729 [details]
Patch

[Charles Wei]

Comment on attachment 149729 [details]
Patch

Thanks for the review,jochen and lvin.

[WebKit Review Bot]

Comment on attachment 149729 [details]
Patch

Clearing flags on attachment: 149729

Committed r121395: <http://trac.webkit.org/changeset/121395>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 90143

[Charles Wei]

Created attachment 149919 [details]
Patch

[Charles Wei]

Created attachment 149924 [details]
Patch

[Charles Wei]

(In reply to comment #41)
> Created an attachment (id=149924) [details]
> Patch

David:  thanks for your comments in Bug #90141.  The latest patch addresses them.

[David Levin]

Comment on attachment 149924 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:97
> +        m_backend->open(name, request.get(), context->securityOrigin(), frame, document->page()->group().groupSettings()->indexedDBDatabasePath());

Do we need to add a check for page() being null here?

> Source/WebCore/workers/DefaultSharedWorkerRepository.cpp:173
> +    return document->page()->group().groupSettings();

Why don't you need to check for page being null here?

> Source/WebCore/workers/WorkerMessagingProxy.cpp:276
> +    Document* document = static_cast<Document*>(m_scriptExecutionContext.get());

You should add an assert that it is a document. When nested workers are supported one day, it would be nice to get asserts in places immediately where there are problems.

ASSERT(m_scriptExecutionContext->isDocument());

> Source/WebCore/workers/WorkerMessagingProxy.cpp:279
> +        settings = document->page()->group().groupSettings();

How do we know if page is not null that group is not null.

> Source/WebKit/chromium/src/WebSharedWorkerImpl.cpp:372
> +    setWorkerThread(SharedWorkerThread::create(name, url, userAgent, static_cast<Document*>(m_loadingDocument.get())->page()->group().groupSettings(),

Why is there no check for page() being null here?

(And add an assert about this being a document.)

> Source/WebKit/chromium/src/WebWorkerClientImpl.cpp:93
> +    GroupSettings* settings = static_cast<Document*>(m_scriptExecutionContext.get())->page()->group().groupSettings();

Why is there no check for page being null here?

[Joshua Bell]

FYI - if https://bugs.webkit.org/show_bug.cgi?id=90310 lands first this will need some manual merging (and vice versa), but it should be pretty straightforward.

[Charles Wei]

Created attachment 150330 [details]
Patch

[David Levin]

Comment on attachment 150330 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:97
> +        m_backend->open(name, request.get(), context->securityOrigin(), frame, document->page()->group().groupSettings()->indexedDBDatabasePath());

Why doesn't document->page() need to be checked here?

> Source/WebCore/workers/DefaultSharedWorkerRepository.cpp:173
> +    if (document->page())

Should this loop through documents if the first one picked happened to have a NULL page?

[David Levin]

btw, I'm only for basically two weeks starting tomorrow. I'm fine with the code modulo these two questions. If another reviewer is satisfied with your response to them, I'm ok if they r+ this change or its revision.  Sorry for any inconvenience.

[Charles Wei]

Comment on attachment 150330 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:97
>> +        m_backend->open(name, request.get(), context->securityOrigin(), frame, document->page()->group().groupSettings()->indexedDBDatabasePath());
> 
> Why doesn't document->page() need to be checked here?

David,  document->page() is already guaranteed to be not null here by early return above (line 94), so we don't need null check here .

>> Source/WebCore/workers/DefaultSharedWorkerRepository.cpp:173
>> +    if (document->page())
> 
> Should this loop through documents if the first one picked happened to have a NULL page?

That's an option, if we decide.

[David Levin]

(In reply to comment #48)
> (From update of attachment 150330 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150330&action=review
> 
> >> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:97
> >> +        m_backend->open(name, request.get(), context->securityOrigin(), frame, document->page()->group().groupSettings()->indexedDBDatabasePath());
> > 
> > Why doesn't document->page() need to be checked here?
> 
> David,  document->page() is already guaranteed to be not null here by early return above (line 94), so we don't need null check here .

Thanks!

> 
> >> Source/WebCore/workers/DefaultSharedWorkerRepository.cpp:173
> >> +    if (document->page())
> > 
> > Should this loop through documents if the first one picked happened to have a NULL page?
> 
> That's an option, if we decide.

Well, when can page be NULL? (I think it means that the document got closed.)

 What does it mean if this information doesn't get passed thru? Because this is what it seems like you'll potentially get at random times and unpredicable which will lead to bugs that people can't reproduce:(

[Charles Wei]

Comment on attachment 150330 [details]
Patch

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

>>>> Source/WebCore/workers/DefaultSharedWorkerRepository.cpp:173
>>>> +    if (document->page())
>>> 
>>> Should this loop through documents if the first one picked happened to have a NULL page?
>> 
>> That's an option, if we decide.
> 
> Well, when can page be NULL? (I think it means that the document got closed.)
> 
>  What does it mean if this information doesn't get passed thru? Because this is what it seems like you'll potentially get at random times and unpredicable which will lead to bugs that people can't reproduce:(

In many cases, a document can be created without a frame, and without a page. 

In this case, if there's no page and no GroupSettings,  the IDB will be created in-memory.

[David Levin]

Comment on attachment 150330 [details]
Patch

Thanks for your patience. Good luck with this time around :).

[Charles Wei]

Comment on attachment 150330 [details]
Patch

Thanks for the review and comments, David. Wish good luck this time :-)

[WebKit Review Bot]

Comment on attachment 150330 [details]
Patch

Clearing flags on attachment: 150330

Committed r121742: <http://trac.webkit.org/changeset/121742>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joshua Bell]

Comment on attachment 150330 [details]
Patch

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

> Source/WebCore/workers/WorkerThread.cpp:198
> +    return m_startupData->m_groupSettings.get();

m_startupData is cleared after the initial script evaluation but before the event loop is executed. This means that any calls to IDBFactory methods that access this (e.g. open) that occur in a callback will dereference null here.

I'm hitting this trying to rebase 90310 on top of this patch; it doesn't look like tests with multiple open() calls from workers were tested against this patch.