Bug 88338

Summary: IndexedDB: should make the LevelDB persistant to the directory indicated in PageGroupSettings::indexedDBDataBasePath
Product: WebKit Reporter: Charles Wei <charles.wei>
Component: WebCore JavaScriptAssignee: Charles Wei <charles.wei>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dgrogan, jochen, jorlow, jrogers, jsbell, levin, rwlbuis, staikos, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 90143    
Bug Blocks: 45110, 90832    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch
none
Patch
none
Patch
none
Patch none

Description Charles Wei 2012-06-05 09:41:19 PDT
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.
Comment 1 Joshua Bell 2012-06-05 09:50:35 PDT
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.
Comment 2 Charles Wei 2012-06-05 18:06:02 PDT
(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.
Comment 3 jochen 2012-06-06 00:52:09 PDT
(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
Comment 4 Charles Wei 2012-06-06 09:36:16 PDT
(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 ?
Comment 5 Charles Wei 2012-06-08 03:00:48 PDT
Created attachment 146528 [details]
Patch
Comment 6 jochen 2012-06-08 03:07:32 PDT
What about just stashing the path into the WorkerContext when the WorkerContext is created?
Comment 7 Joshua Bell 2012-06-08 12:22:33 PDT
(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)
Comment 8 Charles Wei 2012-06-19 20:24:33 PDT
Anybody can help to review ?
Comment 9 jochen 2012-06-20 00:34:50 PDT
(In reply to comment #8)
> Anybody can help to review ?

You haven't yet adressed the old comments?
Comment 10 Charles Wei 2012-06-20 17:55:06 PDT
Created attachment 148693 [details]
Patch
Comment 11 WebKit Review Bot 2012-06-20 19:05:51 PDT
Comment on attachment 148693 [details]
Patch

Attachment 148693 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13010112
Comment 12 Charles Wei 2012-06-20 20:00:56 PDT
Created attachment 148716 [details]
Patch
Comment 13 WebKit Review Bot 2012-06-20 21:06:29 PDT
Comment on attachment 148716 [details]
Patch

Attachment 148716 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13020072
Comment 14 Charles Wei 2012-06-20 21:35:41 PDT
Created attachment 148725 [details]
Patch
Comment 15 WebKit Review Bot 2012-06-20 22:27:16 PDT
Comment on attachment 148725 [details]
Patch

Attachment 148725 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13028025
Comment 16 Charles Wei 2012-06-20 23:06:07 PDT
Created attachment 148735 [details]
Patch
Comment 17 WebKit Review Bot 2012-06-21 00:04:47 PDT
Comment on attachment 148735 [details]
Patch

Attachment 148735 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13039015
Comment 18 Charles Wei 2012-06-21 02:51:03 PDT
Created attachment 148756 [details]
Patch
Comment 19 jochen 2012-06-21 15:01:06 PDT
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?
Comment 20 Charles Wei 2012-06-21 20:30:22 PDT
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?
Comment 21 jochen 2012-06-21 23:38:30 PDT
(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
Comment 22 Charles Wei 2012-06-24 22:23:11 PDT
Created attachment 149239 [details]
Patch
Comment 23 Charles Wei 2012-06-24 22:24:32 PDT
(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 24 jochen 2012-06-25 08:07:31 PDT
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
Comment 25 Charles Wei 2012-06-25 18:50:02 PDT
(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.
Comment 26 jochen 2012-06-26 12:11:42 PDT
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?
Comment 27 Charles Wei 2012-06-26 23:06:08 PDT
Created attachment 149685 [details]
Patch
Comment 28 WebKit Review Bot 2012-06-26 23:18:47 PDT
Comment on attachment 149685 [details]
Patch

Attachment 149685 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13103673
Comment 29 Charles Wei 2012-06-27 01:42:31 PDT
Created attachment 149706 [details]
Patch
Comment 30 jochen 2012-06-27 01:55:49 PDT
(In reply to comment #29)
> Created an attachment (id=149706) [details]
> Patch

This change looks good
Comment 31 Charles Wei 2012-06-27 01:59:28 PDT
(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 :-)
Comment 32 WebKit Review Bot 2012-06-27 02:22:07 PDT
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
Comment 33 WebKit Review Bot 2012-06-27 02:22:11 PDT
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
Comment 34 Charles Wei 2012-06-27 02:35:46 PDT
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.
Comment 35 Charles Wei 2012-06-27 04:45:07 PDT
Created attachment 149729 [details]
Patch
Comment 36 Charles Wei 2012-06-27 17:44:02 PDT
Comment on attachment 149729 [details]
Patch

Thanks for the review,jochen and lvin.
Comment 37 WebKit Review Bot 2012-06-27 18:58:44 PDT
Comment on attachment 149729 [details]
Patch

Clearing flags on attachment: 149729

Committed r121395: <http://trac.webkit.org/changeset/121395>
Comment 38 WebKit Review Bot 2012-06-27 18:58:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 WebKit Review Bot 2012-06-28 01:30:19 PDT
Re-opened since this is blocked by 90143
Comment 40 Charles Wei 2012-06-28 04:06:27 PDT
Created attachment 149919 [details]
Patch
Comment 41 Charles Wei 2012-06-28 04:39:57 PDT
Created attachment 149924 [details]
Patch
Comment 42 Charles Wei 2012-06-28 04:45:44 PDT
(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.
Comment 43 David Levin 2012-06-29 00:32:49 PDT
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?
Comment 44 Joshua Bell 2012-06-29 14:27:38 PDT
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.
Comment 45 Charles Wei 2012-07-01 07:35:27 PDT
Created attachment 150330 [details]
Patch
Comment 46 David Levin 2012-07-02 23:22:18 PDT
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?
Comment 47 David Levin 2012-07-02 23:23:45 PDT
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.
Comment 48 Charles Wei 2012-07-02 23:45:46 PDT
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.
Comment 49 David Levin 2012-07-02 23:47:52 PDT
(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:(
Comment 50 Charles Wei 2012-07-03 01:10:31 PDT
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.
Comment 51 David Levin 2012-07-03 01:11:31 PDT
Comment on attachment 150330 [details]
Patch

Thanks for your patience. Good luck with this time around :).
Comment 52 Charles Wei 2012-07-03 01:19:14 PDT
Comment on attachment 150330 [details]
Patch

Thanks for the review and comments, David. Wish good luck this time :-)
Comment 53 WebKit Review Bot 2012-07-03 01:29:06 PDT
Comment on attachment 150330 [details]
Patch

Clearing flags on attachment: 150330

Committed r121742: <http://trac.webkit.org/changeset/121742>
Comment 54 WebKit Review Bot 2012-07-03 01:29:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 55 Joshua Bell 2012-07-09 15:48:16 PDT
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.