Bug 88338

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.

(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

(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 ?

Created attachment 146528 [details] Patch

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

(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)

Anybody can help to review ?

(In reply to comment #8) > Anybody can help to review ? You haven't yet adressed the old comments?

Created attachment 148693 [details] Patch

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

Created attachment 148716 [details] Patch

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

Created attachment 148725 [details] Patch

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

Created attachment 148735 [details] Patch

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

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?

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

Created attachment 149239 [details] Patch

(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

(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?

Created attachment 149685 [details] Patch

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

Created attachment 149706 [details] Patch

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

(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 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

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 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.

Created attachment 149729 [details] Patch

Comment on attachment 149729 [details] Patch Thanks for the review,jochen and lvin.

Comment on attachment 149729 [details] Patch Clearing flags on attachment: 149729 Committed r121395: <http://trac.webkit.org/changeset/121395>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 90143

Created attachment 149919 [details] Patch

Created attachment 149924 [details] Patch

(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 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?

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.

Created attachment 150330 [details] Patch

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?

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 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.

(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 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 on attachment 150330 [details] Patch Thanks for your patience. Good luck with this time around :).

Comment on attachment 150330 [details] Patch Thanks for the review and comments, David. Wish good luck this time :-)

Comment on attachment 150330 [details] Patch Clearing flags on attachment: 150330 Committed r121742: <http://trac.webkit.org/changeset/121742>

All reviewed patches have been landed. Closing bug.

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.