Bug 73609 – Grant workers experimental access to IndexedDB.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

Allow workers to access IndexedDB.

[David Grogan]

Created attachment 117537 [details]
Patch

[David Grogan]

Created attachment 117542 [details]
Patch

[David Grogan]

Michael/Hans/Josh, any feedback is appreciated.  The chromium side, which has to go in first, is at http://codereview.chromium.org/8747002/.

[Early Warning System Bot]

Comment on attachment 117542 [details]
Patch

Attachment 117542 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10704869

[David Grogan]

Created attachment 117544 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117544 [details]
Patch

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

[David Grogan]

Created attachment 117553 [details]
Patch

[David Levin]

Comment on attachment 117553 [details]
Patch

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

A few questions...

> Source/WebKit/chromium/ChangeLog:3
> +        Grant workers experimental access to IndexedDB.

What do you mean experimental access?

> Source/WebCore/storage/IDBFactory.cpp:101
> +    m_backend->openFromWorker(name, request, context->securityOrigin(), static_cast<WorkerContext*>(context), String());

Why have two methods open/openFromWorker when they have the same impl?


Get rid of that and a lot of this code collapses.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:106
> +void IDBFactoryBackendImpl::openFromWorker(const String& name, PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<SecurityOrigin> securityOrigin, WorkerContext*, const String& dataDir)

please avoid abbreviations: "dataDir"

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:158
> +{

Why is "callbacks" a PassRefPtr when nothing takes ownership of it in here?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:159
> +    const String uniqueIdentifier = computeUniqueIdentifier(name, securityOrigin.get());

securityOrigin should be put in a local RefPtr since you are doing more than just passing it directly to another function call.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:178
> +    m_databaseBackendMap.set(uniqueIdentifier, databaseBackend.get());

How does the lifetime of databaseBackend work?

It looks like it goes out of scope and nothing has a ref count on it.

Yes, I know this is the code that existed before :).

> Source/WebCore/workers/WorkerContext.cpp:532
> +        m_idbFactoryBackendInterface = IDBFactoryBackendInterface::create().get();

You shouldn't need to do "get" here.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:110
> +    void signalCompleted(bool result)

Why isn't "result" used?

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:115
> +                createCallbackTask(&didComplete, AllowCrossThreadAccess(this), result), m_mode);

AllowCrossThreadAccess is a really really bad sign.

Since "this" is a ThreadSafeRefCounted object, you shouldn't need to use it.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:121
> +        , m_mode(mode)

why isn't m_result initialized?

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:125
> +                               AllowCrossThreadAccess(frame),

Everytime you put in AllowCrossThreadAccess, you need to be able to answer the question: How does this stay alive until it is used in the callback?

Please answer for commonClient and frame.  Also remove it for "this".

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:149
> +    WebCore::WorkerLoaderProxy* m_workerLoaderProxy;

WebCore:: shouldn't be needed anywhere due to the using statement above.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:150
> +    WTF::String m_mode;

WTF shouldn't be needed iirc.

Even more critical, putting a String (a RefCounted object) in a ThreadSafeRefCounted object is a no-no. Please find another way to accomplish your goal.

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:155
> +    WorkerContext* workerContext = static_cast<WorkerContext*>(context);

Assert that the context is a worker first. (I feel like we should have a function that does this cast and assert but I guess we don't)

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:158
> +    WorkerLoaderProxy* workerLoaderProxy = &workerContext->thread()->workerLoaderProxy();

Why the loader proxy? (This looks odd to me.)

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:169
> +    if (runLoop.runInMode(workerContext, mode) == MessageQueueTerminated) {

There is only a single task posted to this mode?

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:187
> +    m_webIDBFactory->open(name, new WebIDBCallbacksImpl(callbacks), origin, webFrame, dataDir);

A raw new is a bad sign. Who owns this? Why doesn't it have an adoptPtr around it?

[David Grogan]

Created attachment 117596 [details]
Patch

[David Grogan]

Comment on attachment 117553 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:3
>> +        Grant workers experimental access to IndexedDB.
> 
> What do you mean experimental access?

That it has rough edges, it's most likely buggy, etc.  Even with these caveats there is demand for it.  In m17 :/

>> Source/WebCore/storage/IDBFactory.cpp:101
>> +    m_backend->openFromWorker(name, request, context->securityOrigin(), static_cast<WorkerContext*>(context), String());
> 
> Why have two methods open/openFromWorker when they have the same impl?
> 
> 
> Get rid of that and a lot of this code collapses.

They have the same impl in the browser process, IDBFactoryBackendImpl, but not in IDBFactoryBackendProxy, the renderer side.

>> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:106
>> +void IDBFactoryBackendImpl::openFromWorker(const String& name, PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<SecurityOrigin> securityOrigin, WorkerContext*, const String& dataDir)
> 
> please avoid abbreviations: "dataDir"

Fixed.

>> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:158
>> +{
> 
> Why is "callbacks" a PassRefPtr when nothing takes ownership of it in here?

I don't know why it was originally.  Changed to raw.

>> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:159
>> +    const String uniqueIdentifier = computeUniqueIdentifier(name, securityOrigin.get());
> 
> securityOrigin should be put in a local RefPtr since you are doing more than just passing it directly to another function call.

Done.

>> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:178
>> +    m_databaseBackendMap.set(uniqueIdentifier, databaseBackend.get());
> 
> How does the lifetime of databaseBackend work?
> 
> It looks like it goes out of scope and nothing has a ref count on it.
> 
> Yes, I know this is the code that existed before :).

The onSuccess call in the line above this one constructs a PassRefPtr.  When I try to change
callbacks->onSuccess(databaseBackend.get());
to
callbacks->onSuccess(databaseBackend.release());
I get an ambiguity compiler error.  It seems that the workarounds are just as unclear as this original code.

RefPtr<IDBDatabaseBackendImpl> objects live inside WebIDBDatabaseImpl objects, which are stored in an IDMap in chromium, here:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/browser/in_process_webkit/indexed_db_dispatcher_host.h&type=cs&l=134

>> Source/WebCore/workers/WorkerContext.cpp:532
>> +        m_idbFactoryBackendInterface = IDBFactoryBackendInterface::create().get();
> 
> You shouldn't need to do "get" here.

Removed.

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:110
>> +    void signalCompleted(bool result)
> 
> Why isn't "result" used?

It's passed to createCallbackTask below, which passes it on to didComplete.

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:115
>> +                createCallbackTask(&didComplete, AllowCrossThreadAccess(this), result), m_mode);
> 
> AllowCrossThreadAccess is a really really bad sign.
> 
> Since "this" is a ThreadSafeRefCounted object, you shouldn't need to use it.

Removed AllowCrossThreadAccess from this.

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:121
>> +        , m_mode(mode)
> 
> why isn't m_result initialized?

It's only read after it's set.  I'll add it though.

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:125
>> +                               AllowCrossThreadAccess(frame),
> 
> Everytime you put in AllowCrossThreadAccess, you need to be able to answer the question: How does this stay alive until it is used in the callback?
> 
> Please answer for commonClient and frame.  Also remove it for "this".

The commonClient check was moved back to the worker thread, so it is no longer needed.

As for frame, I see what you mean, there's no guarantee the pointer hasn't become invalid.  I'll look for some indicator other than the worker's runloop terminating to listen for.  It looks like the corresponding code for WebSQLDatabase passes frame over to the main thread with AllowCrossThreadAccess.  Though I'm not sure if that indicates that this is safe or if it's just also unsafe.

I also considered stashing whether IDB is allowed when a worker is created but didn't see anywhere very obvious to do so.

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:149
>> +    WebCore::WorkerLoaderProxy* m_workerLoaderProxy;
> 
> WebCore:: shouldn't be needed anywhere due to the using statement above.

Fixed.  That's what you get when you copy and paste code.

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:150
>> +    WTF::String m_mode;
> 
> WTF shouldn't be needed iirc.
> 
> Even more critical, putting a String (a RefCounted object) in a ThreadSafeRefCounted object is a no-no. Please find another way to accomplish your goal.

Replaced with AtomicString.  Let me know if that's insufficient.

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:155
>> +    WorkerContext* workerContext = static_cast<WorkerContext*>(context);
> 
> Assert that the context is a worker first. (I feel like we should have a function that does this cast and assert but I guess we don't)

I moved the assert and cast earlier in the callstack.

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:158
>> +    WorkerLoaderProxy* workerLoaderProxy = &workerContext->thread()->workerLoaderProxy();
> 
> Why the loader proxy? (This looks odd to me.)

It offers a convenient way for the main thread to post a task back to the worker thread.  We can also get the webFrame and webView from it.

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:169
>> +    if (runLoop.runInMode(workerContext, mode) == MessageQueueTerminated) {
> 
> There is only a single task posted to this mode?

That's the idea.

I can't tell if you're asking or quizzing :)

>> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:187
>> +    m_webIDBFactory->open(name, new WebIDBCallbacksImpl(callbacks), origin, webFrame, dataDir);
> 
> A raw new is a bad sign. Who owns this? Why doesn't it have an adoptPtr around it?

It's owned by an IDMap in chromium.  Which is why it can't have the adoptPtr.  It seems that whenever an object is passed from webkit to chromium there's no way around using a raw new.  Is that accurate?

[Hans Wennborg]

Comment on attachment 117596 [details]
Patch

I only noticed two little nits..

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

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:115
> +                createCallbackTask(&didComplete, this, result), m_mode);

no need for linebreak

> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:127
> +            createCallbackTask(&allowIndexedDBTask, this, AllowCrossThreadAccess(webFrame), name));

no need for linebreak

[David Levin]

(In reply to comment #10)
> (From update of attachment 117553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117553&action=review


> 
> >> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:178
> >> +    m_databaseBackendMap.set(uniqueIdentifier, databaseBackend.get());
> > 
> > How does the lifetime of databaseBackend work?
> > 
> > It looks like it goes out of scope and nothing has a ref count on it.
> > 
> > Yes, I know this is the code that existed before :).
> 
> The onSuccess call in the line above this one constructs a PassRefPtr.  When I try to change
> callbacks->onSuccess(databaseBackend.get());
> to
> callbacks->onSuccess(databaseBackend.release());
> I get an ambiguity compiler error.  It seems that the workarounds are just as unclear as this original code.

Don't do release because the next line uses it :).

I think you need something like this: RefPtr<IDBDatabaseBackendInterface>(databaseBackend.get()).release()

which looks odd but will properly convey what is going on.






> 
> RefPtr<IDBDatabaseBackendImpl> objects live inside WebIDBDatabaseImpl objects, which are stored in an IDMap in chromium, here:
> http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/browser/in_process_webkit/indexed_db_dispatcher_host.h&type=cs&l=134


> 
> >> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:125
> >> +                               AllowCrossThreadAccess(frame),
> > 
> > Everytime you put in AllowCrossThreadAccess, you need to be able to answer the question: How does this stay alive until it is used in the callback?
> > 
> > Please answer for commonClient and frame.  Also remove it for "this".
> 
> The commonClient check was moved back to the worker thread, so it is no longer needed.
> 
> As for frame, I see what you mean, there's no guarantee the pointer hasn't become invalid.  I'll look for some indicator other than the worker's runloop terminating to listen for.  It looks like the corresponding code for WebSQLDatabase passes frame over to the main thread with AllowCrossThreadAccess.  Though I'm not sure if that indicates that this is safe or if it's just also unsafe.

You can look back at the bug that went in with and see if lifetime was discussed. If so, you're in luck. If not, I would be concerned.

Once you figure out your instance, perhaps you could add a comment in the code to explain your instance.


> 
> I also considered stashing whether IDB is allowed when a worker is created but didn't see anywhere very obvious to do so.
> 



> >> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:150
> >> +    WTF::String m_mode;
> > 
> > WTF shouldn't be needed iirc.
> > 
> > Even more critical, putting a String (a RefCounted object) in a ThreadSafeRefCounted object is a no-no. Please find another way to accomplish your goal.
> 
> Replaced with AtomicString.  Let me know if that's insufficient.

AtomicString is even worse. AtomicString is hard bound to a particular thread due to using tls. If it gets destroyed on the wrong thread, I think it is pretty guaranteed to do something bad iirc.

It is best to factor this such that a string isn't needed in the class.

For example can you pass the mode in a message (i.e. createCallbackTask) between the threads and not have it in the class?
Can you factor the class such that the string is in a new class which is only ever used on one thread?


I just saw that IDBDatabaseBackendImpl has strings in it and is ThreadSafeRefCounted. This class is way way too complicated to be ThreadSafeRefCounted imo. (String's don't appear to be the only problem issue in it.)




> >> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:169
> >> +    if (runLoop.runInMode(workerContext, mode) == MessageQueueTerminated) {
> > 
> > There is only a single task posted to this mode?
> 
> That's the idea.
> 
> I can't tell if you're asking or quizzing :)

No sure what you mean. I thought this was true but was checking to be sure.


> >> Source/WebKit/chromium/src/IDBFactoryBackendProxy.cpp:187
> >> +    m_webIDBFactory->open(name, new WebIDBCallbacksImpl(callbacks), origin, webFrame, dataDir);
> > 
> > A raw new is a bad sign. Who owns this? Why doesn't it have an adoptPtr around it?
> 
> It's owned by an IDMap in chromium.  Which is why it can't have the adoptPtr.  It seems that whenever an object is passed from webkit to chromium there's no way around using a raw new.  Is that accurate?

Hmm... for now :).

[David Grogan]

In hopes of getting this into m17, instead of monkeying around with making the bridge threadsafe, I'm going to put IndexedDB-on-workers behind a runtime flag defaulted to false.  When that flag is set to true we can bypass the content settings check.

[David Levin]

(In reply to comment #13)
> In hopes of getting this into m17, instead of monkeying around with making the bridge threadsafe, I'm going to put IndexedDB-on-workers behind a runtime flag defaulted to false.  When that flag is set to true we can bypass the content settings check.

So you want to check in obviously broken code that may corrupt memory if it is used?

[David Grogan]

(In reply to comment #14)
> (In reply to comment #13)
> > In hopes of getting this into m17, instead of monkeying around with making the bridge threadsafe, I'm going to put IndexedDB-on-workers behind a runtime flag defaulted to false.  When that flag is set to true we can bypass the content settings check.
> 
> So you want to check in obviously broken code that may corrupt memory if it is used?

No, the exact opposite, bypassing the content settings check would allow me to remove all this code.

[David Grogan]

Created attachment 117668 [details]
Patch

[David Grogan]

Comment on attachment 117668 [details]
Patch

This, combined with changing the default value of the flag in chromium is what I had in mind.

I removed the ability to disable idb at runtime, something that only chromium uses and has had set to true for a long time.

[David Levin]

(In reply to comment #17)
> (From update of attachment 117668 [details])
> This, combined with changing the default value of the flag in chromium is what I had in mind.
> 
> I removed the ability to disable idb at runtime, something that only chromium uses and has had set to true for a long time.

Thanks.

Threading issues in this particular patch seem ok. (I'll leave it to others to verify overall idea.)

[David Grogan]

Nate, sorry for the crunch, but would you be able to review this today?

Dave Levin has taken a few looks at this already but won't be able to finish it today.  Greg (Simon) said he's ok with putting this behind a flag.

[Nate Chapin]

Comment on attachment 117668 [details]
Patch

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

Per comment #18, I'm taking on faith that this is correct from a threading perspective.

Otherwise, this seems sane.

> Source/WebCore/storage/IDBFactory.cpp:85
> +    ASSERT(context->isDocument() || context->isWorkerContext());

Is this ASSERT actually necessary? I thought these were the only 2 types of ScriptExecutionContexts

[David Grogan]

I put it in just to alert if a new cast is ever needed, if there becomes a new type of context.  Might be silly.

[WebKit Review Bot]

Comment on attachment 117668 [details]
Patch

Clearing flags on attachment: 117668

Committed r101890: <http://trac.webkit.org/changeset/101890>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Grogan]

*** Bug 56787 has been marked as a duplicate of this bug. ***