Bug 40054 – [indexedDB] It is impossible to create object stores

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andrei Popescu]

The implementation of IDBDatabase::createObjectStore() is stubbed out. Patch to fix this coming very soon.

[Andrei Popescu]

Created attachment 57652 [details]
Patch

[Andrei Popescu]

Created attachment 57653 [details]
Patch

[Andrei Popescu]

Created attachment 57655 [details]
Patch

[Jeremy Orlow]

Here's the comments I had queued up from you last patch:


WebCore/WebCore.gypi:1691
 +              'inspector/InspectorFrontendClientLocal.cpp',
hu?

WebCore/WebCore.gypi: 
 +              'inspector/front-end/HAREntry.js',
?

WebCore/GNUmakefile.am:2522
 +  	WebCore/storage/IDBWeakFramePtr.h \
Add to gypi, pro, and vcproj


WebCore/storage/IDBDatabaseImpl.cpp:37
 +  IDBDatabaseImpl::IDBDatabaseImpl(const String& name, const String& description, const String& version, PassRefPtr<IDBWeakFramePtr> framePtr)
Passing via * is slightly better here.  See http://webkit.org/coding/RefPtr.html

WebCore/storage/IDBDatabaseImpl.h:40
 +      static PassRefPtr<IDBDatabase> create(const String& name, const String& description, const String& version, PassRefPtr<IDBWeakFramePtr> framePtr)
ditto

WebCore/storage/IDBDatabaseRequest.cpp:52
 +      if (!(*m_idbDatabase->weakFramePtr()))
Maybe we should add a .get()?  if (!blahblah().get()) seems nicer.

WebCore/storage/IDBDatabaseRequest.cpp:53
 +          return 0;
Is this going to be handled gracefully by the caller?  I doubt it.  I'd create the request object either way and return it (without calling createObjectStore...though that's just an optimization...make that clear in a comment).  Since we're going away, it's no big deal that neither the onsuccess nor the onerror will ever be fired.

WebCore/storage/IDBDatabaseRequest.h:39
 +  class IDBRequest;
Alpha order.

WebCore/storage/IDBObjectStore.h:42
 +  // FIXME: This needs to be split into an interface and Impl classes.
Remove

WebCore/storage/IDBObjectStoreImpl.cpp:51
 +      RefPtr<DOMStringList> indexNames = DOMStringList::create();
I don't think we need to store a DOMStringList.  Can't you just have a vector of IDBIndexImpls and query each of those for its name on demand?  It doesn't seem worth the effort of keeping multiple lists up to date at once.  You can do this in a future change if you'd like.

[Jeremy Orlow]

Comment on attachment 57655 [details]
Patch

WebKit/chromium/src/WebIDBObjectStoreImpl.h:13
 +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
Get rid of #3

WebKit/chromium/src/WebIDBObjectStoreImpl.h:48
 +      virtual WebString keyPath() const;
newline after this...also put in a fixme for anything you didn't implement (like indexNames)

[WebKit Review Bot]

Attachment 57653 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2846018

[Jeremy Orlow]

Comment on attachment 57655 [details]
Patch

WebKit/chromium/src/WebIDBDatabaseImpl.h: 
 +  
this newline is probably OK...if you want.

WebKit/chromium/src/WebIDBDatabaseImpl.h:48
 +      virtual void createObjectStore(const WebString& name, const WebString& keyPath, bool autoIncrement, WebIDBCallbacks* callbacks, WebFrame*);
Newline before private:

WebKit/chromium/src/WebIDBCallbacksImpl.cpp:69
 +      m_callbacks->onSuccess(IDBDatabaseProxy::create(webKitInstance, weakFramePtr.release()));
I don't really see a reason not to combine these lines.

WebKit/chromium/src/IndexedDatabaseProxy.h:45
 +      virtual void open(const String& name, const String& description, PassRefPtr<IDBCallbacks>, PassRefPtr<SecurityOrigin>, PassRefPtr<IDBWeakFramePtr>);
This could be a *IDBWeakFramePtr I believe.

WebKit/chromium/src/IndexedDatabaseProxy.cpp:62
 +      Frame* frame = (*framePtr).get();
framePtr->

WebKit/chromium/src/IDBObjectStoreProxy.h:13
 +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
Remove

WebKit/chromium/src/IDBObjectStoreProxy.h:57
 +  private:
newline before?

WebKit/chromium/src/IDBObjectStoreProxy.cpp:13
 +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
remove

WebKit/chromium/src/IDBDatabaseProxy.h:44
 +      static PassRefPtr<IDBDatabase> create(PassOwnPtr<WebKit::WebIDBDatabase>, PassRefPtr<IDBWeakFramePtr>);
Just pass pointers.

WebCore/storage/IDBDatabaseImpl.h:60
 +      RefPtr<IDBWeakFramePtr> m_weakFramePtr;
I don't think it makes sense for this to be here.  The frame pointer is really an issue that should be entirely constrained to the request objects.

WebCore/storage/IDBDatabaseRequest.h:56
 +      PassRefPtr<IDBRequest> createObjectStore(const String& name, const String& keyPath = "", bool autoIncrement = false);
newline after

WebCore/storage/IDBWeakFramePtr.h:49
 +      bool operator!() const {return !m_frame; }
Wait...doesn't this mean you can just do |if (!weakFramePtr)| since the ref ptr overloads that operator and this overloads it as well?

WebCore/storage/IndexedDatabaseRequest.h:57
 +      void disconnectFrame() { m_framePtr->disconnectFrame(); }
I think you should get rid of disconnectFrame here, have DOMWindow create the IDBWeakFramePtr on demand, and call disconnectFrame direclty on it when necessary.

[Andrei Popescu]

Thanks for the review Jeremy!

(In reply to comment #4)
> Here's the comments I had queued up from you last patch:
> 
> 
> WebCore/WebCore.gypi:1691
>  +              'inspector/InspectorFrontendClientLocal.cpp',
> hu?
> 
> WebCore/WebCore.gypi: 
>  +              'inspector/front-end/HAREntry.js',
> ?
> 

Silly, I know. Fixed.

> WebCore/GNUmakefile.am:2522
>  +      WebCore/storage/IDBWeakFramePtr.h \
> Add to gypi, pro, and vcproj
> 

Added.

> 
> WebCore/storage/IDBDatabaseImpl.cpp:37
>  +  IDBDatabaseImpl::IDBDatabaseImpl(const String& name, const String& description, const String& version, PassRefPtr<IDBWeakFramePtr> framePtr)
> Passing via * is slightly better here.  See http://webkit.org/coding/RefPtr.html
>

Agreed. Done.
 
> WebCore/storage/IDBDatabaseImpl.h:40
>  +      static PassRefPtr<IDBDatabase> create(const String& name, const String& description, const String& version, PassRefPtr<IDBWeakFramePtr> framePtr)
> ditto
>

ditto
 
> WebCore/storage/IDBDatabaseRequest.cpp:52
>  +      if (!(*m_idbDatabase->weakFramePtr()))
> Maybe we should add a .get()?  if (!blahblah().get()) seems nicer.
>

It already had a get() but I just preferred the other way. But ok, if you think it's nicer.
 
> WebCore/storage/IDBDatabaseRequest.cpp:53
>  +          return 0;
> Is this going to be handled gracefully by the caller?  I doubt it.  I'd create the request object either way and return it (without calling createObjectStore...though that's just an optimization...make that clear in a comment).  Since we're going away, it's no big deal that neither the onsuccess nor the onerror will ever be fired.
>

As discussed, this seems reasonable for now. The frame is going away so I am not sure doing something else makes more sense.
 
> WebCore/storage/IDBDatabaseRequest.h:39
>  +  class IDBRequest;
> Alpha order.
> 

Done.

> WebCore/storage/IDBObjectStore.h:42
>  +  // FIXME: This needs to be split into an interface and Impl classes.
> Remove
>

Removed.
 
> WebCore/storage/IDBObjectStoreImpl.cpp:51
>  +      RefPtr<DOMStringList> indexNames = DOMStringList::create();
> I don't think we need to store a DOMStringList.  Can't you just have a vector of IDBIndexImpls and query each of those for its name on demand?  It doesn't seem worth the effort of keeping multiple lists up to date at once.  You can do this in a future change if you'd like.

Hmm, that is your code, I just moved it to the Impl class :)

[Andrei Popescu]

(In reply to comment #5)
> (From update of attachment 57655 [details])
> WebKit/chromium/src/WebIDBObjectStoreImpl.h:13
>  +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> Get rid of #3
> 

Got rid.

> WebKit/chromium/src/WebIDBObjectStoreImpl.h:48
>  +      virtual WebString keyPath() const;
> newline after this...also put in a fixme for anything you didn't implement (like indexNames)

Added.

[Andrei Popescu]

(In reply to comment #7)
> (From update of attachment 57655 [details])
> WebKit/chromium/src/WebIDBDatabaseImpl.h: 
>  +  
> this newline is probably OK...if you want.
> 

I want.

> WebKit/chromium/src/WebIDBDatabaseImpl.h:48
>  +      virtual void createObjectStore(const WebString& name, const WebString& keyPath, bool autoIncrement, WebIDBCallbacks* callbacks, WebFrame*);
> Newline before private:
>

Added.
 
> WebKit/chromium/src/WebIDBCallbacksImpl.cpp:69
>  +      m_callbacks->onSuccess(IDBDatabaseProxy::create(webKitInstance, weakFramePtr.release()));
> I don't really see a reason not to combine these lines.
>

Indeed, especially now that we pass a raw pointer.
 
> WebKit/chromium/src/IndexedDatabaseProxy.h:45
>  +      virtual void open(const String& name, const String& description, PassRefPtr<IDBCallbacks>, PassRefPtr<SecurityOrigin>, PassRefPtr<IDBWeakFramePtr>);
> This could be a *IDBWeakFramePtr I believe.
>

Done.
 
> WebKit/chromium/src/IndexedDatabaseProxy.cpp:62
>  +      Frame* frame = (*framePtr).get();
> framePtr->
>

Done.
 
> WebKit/chromium/src/IDBObjectStoreProxy.h:13
>  +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> Remove
>

Removed.
 
> WebKit/chromium/src/IDBObjectStoreProxy.h:57
>  +  private:
> newline before?
>

Added.
 
> WebKit/chromium/src/IDBObjectStoreProxy.cpp:13
>  +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> remove
>

Removed.
 
> WebKit/chromium/src/IDBDatabaseProxy.h:44
>  +      static PassRefPtr<IDBDatabase> create(PassOwnPtr<WebKit::WebIDBDatabase>, PassRefPtr<IDBWeakFramePtr>);
> Just pass pointers.
>

Done.
 
> WebCore/storage/IDBDatabaseImpl.h:60
>  +      RefPtr<IDBWeakFramePtr> m_weakFramePtr;
> I don't think it makes sense for this to be here.  The frame pointer is really an issue that should be entirely constrained to the request objects.
>

Yes, but the IDBDatabase needs to be able to create Request objects of its own, so it need to pass the weak reference to them.

 
> WebCore/storage/IDBDatabaseRequest.h:56
>  +      PassRefPtr<IDBRequest> createObjectStore(const String& name, const String& keyPath = "", bool autoIncrement = false);
> newline after
>

Added.
 
> WebCore/storage/IDBWeakFramePtr.h:49
>  +      bool operator!() const {return !m_frame; }
> Wait...doesn't this mean you can just do |if (!weakFramePtr)| since the ref ptr overloads that operator and this overloads it as well?
>

Unfortunately not. The ! operator applies to the object, not to a pointer to an object. So when you apply it to the RefPtr, it will test its m_ptr pointer against NULL. It will not apply the ! operator to the object pointed by m_ptr.

> WebCore/storage/IndexedDatabaseRequest.h:57
>  +      void disconnectFrame() { m_framePtr->disconnectFrame(); }
> I think you should get rid of disconnectFrame here, have DOMWindow create the IDBWeakFramePtr on demand, and call disconnectFrame direclty on it when necessary.

I am not sure I agree. Right now, the weak pointer is a private implementation detail of the IDB module. The entry point to that is the IndexedDatabaseRequest class so I think it is neater to let the DOMWindow only know about the IndexedDatabaseRequest instance. Perhaps we should reconsider this if we are to use the weak pointer idea in other places?

Thanks,
Andrei

[Jeremy Orlow]

(In reply to comment #10)
> (In reply to comment #7)
> > WebCore/storage/IDBDatabaseImpl.h:60
> >  +      RefPtr<IDBWeakFramePtr> m_weakFramePtr;
> > I don't think it makes sense for this to be here.  The frame pointer is really an issue that should be entirely constrained to the request objects.
> >
> 
> Yes, but the IDBDatabase needs to be able to create Request objects of its own, so it need to pass the weak reference to them.

We need to figure out another way.  In Chromium, these pointers don't exist in the browser process which is where the impl exists.  I know you're hijacking them at the proxy layer, but it's still pretty ugly.  Lets figure out a way.

> > WebCore/storage/IndexedDatabaseRequest.h:57
> >  +      void disconnectFrame() { m_framePtr->disconnectFrame(); }
> > I think you should get rid of disconnectFrame here, have DOMWindow create the IDBWeakFramePtr on demand, and call disconnectFrame direclty on it when necessary.
> 
> I am not sure I agree. Right now, the weak pointer is a private implementation detail of the IDB module. The entry point to that is the IndexedDatabaseRequest class so I think it is neater to let the DOMWindow only know about the IndexedDatabaseRequest instance. Perhaps we should reconsider this if we are to use the weak pointer idea in other places?

Is it possible for IndexedDatabaseRequest to go away before disconnectFrame is called?  If so, we have a problem.  If not then add an assert into ~IndexedDatabaseRequest to ensure the weak pointer is 0.  That'll alleviate my worry if so.

[Andrei Popescu]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #7)
> > > WebCore/storage/IDBDatabaseImpl.h:60
> > >  +      RefPtr<IDBWeakFramePtr> m_weakFramePtr;
> > > I don't think it makes sense for this to be here.  The frame pointer is really an issue that should be entirely constrained to the request objects.
> > >
> > 
> > Yes, but the IDBDatabase needs to be able to create Request objects of its own, so it need to pass the weak reference to them.
> 
> We need to figure out another way.  In Chromium, these pointers don't exist in the browser process which is where the impl exists.  I know you're hijacking them at the proxy layer, but it's still pretty ugly.  Lets figure out a way.
> 

I filed 40071 about this.

> > > WebCore/storage/IndexedDatabaseRequest.h:57
> > >  +      void disconnectFrame() { m_framePtr->disconnectFrame(); }
> > > I think you should get rid of disconnectFrame here, have DOMWindow create the IDBWeakFramePtr on demand, and call disconnectFrame direclty on it when necessary.
> > 
> > I am not sure I agree. Right now, the weak pointer is a private implementation detail of the IDB module. The entry point to that is the IndexedDatabaseRequest class so I think it is neater to let the DOMWindow only know about the IndexedDatabaseRequest instance. Perhaps we should reconsider this if we are to use the weak pointer idea in other places?
> 
> Is it possible for IndexedDatabaseRequest to go away before disconnectFrame is called?  If so, we have a problem.  If not then add an assert into ~IndexedDatabaseRequest to ensure the weak pointer is 0.  That'll alleviate my worry if so.

Ok, I will add the assert.

[Jeremy Orlow]

Andrei, I'm sorry for not realizing this sooner, but all we need to do for IndexedDB is annotate all of our function that return an IDBRequest object with [CallWith=ScriptExecutionContext] and then pass that into the IDBRequest when we're creating it.  It's already an ActiveDOMObject, so it knows when the script execution context goes away.  I'm not sure why this didn't occur to me sooner.  Sorry for the run around.  :-(

For IndexedDatabaseRequest, instead of passing the Frame* on instantiation, we should probably try to derive the frame from the scriptExecutionContext (see if it's a document, and if so cast...otherwise ASSERT and put a FIXME: Make this work within workers) and pass that in instead.  This can be done in a subsequent patch if you'd rather, though it's easy enough you might just want to do it here?

[Andrei Popescu]

Created attachment 57760 [details]
Patch

[Andrei Popescu]

Created attachment 57762 [details]
Patch

[Jeremy Orlow]

Comment on attachment 57762 [details]
Patch

WebCore/ChangeLog:12
 +          * Android.mk:
This list will need to be updated before you land.

WebCore/bindings/scripts/CodeGeneratorV8.pm:2537
 +              $result .= $indent . "if (!scriptContext)\n";
In what cases do we expect this to be possible?

WebCore/page/DOMWindow.cpp:475
 +      m_indexedDatabaseRequest = 0;
Is this necessary?  I can't think of why it would be.

WebCore/storage/IDBDatabaseImpl.cpp:56
 +      callbacks->onSuccess(objectStore.release());
You need to keep a list of object stores and add it to that.  And if it's already there, return an error.  At least add a FIXME now.

WebCore/storage/IDBDatabaseRequest.cpp:52
 +          // FIXME: make this work with workers.
I think just an assert is fine.  Sure it'll crash and burn badly if we're in release mode, but it'd be hard for someone to accidentally enable in workers.  :-)

WebCore/storage/IDBDatabaseRequest.cpp:58
 +          return 0;
I think you should still return the IDBRequest and just never call onsuccess/onerror since it's going away anyway.  Let's not create weird bugs for developers that are impossible to debug.

WebCore/storage/IDBObjectStoreImpl.h:45
 +      String name() const { return m_name; }
const Sring&

WebCore/storage/IDBObjectStoreImpl.h:46
 +      String keyPath() const { return m_keyPath; }
ditto

WebCore/storage/IDBObjectStoreImpl.h:59
 +      typedef HashMap<String, RefPtr<IDBIndex> > IndexMap;
maybe a newline before typedef?

WebCore/storage/IDBRequest.h:71
 +      // FIXME: Have one onSuccess function for each possible result type.
I think we can remove this now...should be obvious.

WebCore/storage/IndexedDatabaseRequest.cpp:56
 +      if (!context->isDocument()) {
same things here as above.

WebKit/chromium/public/WebIDBDatabase.h:62
 +      virtual void createObjectStore(const WebString& name, const WebString& keyPath, bool autoIncrement, WebIDBCallbacks*, WebFrame*)
This doesn't need the frame.  Only IndexedDatabase::open since we'll need to check the content settings.

WebKit/chromium/public/WebIDBObjectStore.h:13
 +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
delete

WebKit/chromium/src/IDBObjectStoreProxy.cpp:66
 +      return 0;
Assert not implemented

WebKit/chromium/src/IDBObjectStoreProxy.cpp:71
 +      // FIXME: implement.
Why not do this now?  Just add it to the Web interface and redirect...no sense in doing part of the plumbing but not all of it.


Close...I should probably do one more scan through though.

[Jeremy Orlow]

Nate, can you please look at the code generator part of this?

[Andrei Popescu]

(In reply to comment #16)
> (From update of attachment 57762 [details])
> WebCore/ChangeLog:12
>  +          * Android.mk:
> This list will need to be updated before you land.
> 

Will do.

> WebCore/bindings/scripts/CodeGeneratorV8.pm:2537
>  +              $result .= $indent . "if (!scriptContext)\n";
> In what cases do we expect this to be possible?
>

Not entirely sure, but that is the pattern elsewhere in the bindings where the ScriptExecutionContext is needed.

> WebCore/page/DOMWindow.cpp:475
>  +      m_indexedDatabaseRequest = 0;
> Is this necessary?  I can't think of why it would be.
> 

But DOMWindow::clear() sets all other ref pointers to 0, so the expectation is that all DOMWindow objects are no longer accessible once clear() is called. Why would indexed database be an exception?

> WebCore/storage/IDBDatabaseImpl.cpp:56
>  +      callbacks->onSuccess(objectStore.release());
> You need to keep a list of object stores and add it to that.  And if it's already there, return an error.  At least add a FIXME now.
> 

Ah yes, will add.

> WebCore/storage/IDBDatabaseRequest.cpp:52
>  +          // FIXME: make this work with workers.
> I think just an assert is fine.  Sure it'll crash and burn badly if we're in release mode, but it'd be hard for someone to accidentally enable in workers.  :-)

Ok.

> 
> WebCore/storage/IDBDatabaseRequest.cpp:58
>  +          return 0;
> I think you should still return the IDBRequest and just never call onsuccess/onerror since it's going away anyway.  Let's not create weird bugs for developers that are impossible to debug.
>

Hmm, but we discussed that yesterday and agreed that returning null is better than making it look as if the call succeeded but then failing to fire any of the events. Either way, this should be in the spec.
 
> WebCore/storage/IDBObjectStoreImpl.h:45
>  +      String name() const { return m_name; }
> const Sring&
> 
> WebCore/storage/IDBObjectStoreImpl.h:46
>  +      String keyPath() const { return m_keyPath; }
> ditto
> 
> WebCore/storage/IDBObjectStoreImpl.h:59
>  +      typedef HashMap<String, RefPtr<IDBIndex> > IndexMap;
> maybe a newline before typedef?
> 
> WebCore/storage/IDBRequest.h:71
>  +      // FIXME: Have one onSuccess function for each possible result type.
> I think we can remove this now...should be obvious.
> 

Will fix all of the above.

> WebCore/storage/IndexedDatabaseRequest.cpp:56
>  +      if (!context->isDocument()) {
> same things here as above.
> 
> WebKit/chromium/public/WebIDBDatabase.h:62
>  +      virtual void createObjectStore(const WebString& name, const WebString& keyPath, bool autoIncrement, WebIDBCallbacks*, WebFrame*)
> This doesn't need the frame.  Only IndexedDatabase::open since we'll need to check the content settings.
> 

Ok, I could not remember what exactly were we going to do with the WebFrame, so I left it in. Will remove it.

> WebKit/chromium/public/WebIDBObjectStore.h:13
>  +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> delete
>
 
> WebKit/chromium/src/IDBObjectStoreProxy.cpp:66
>  +      return 0;
> Assert not implemented
> 
> WebKit/chromium/src/IDBObjectStoreProxy.cpp:71
>  +      // FIXME: implement.
> Why not do this now?  Just add it to the Web interface and redirect...no sense in doing part of the plumbing but not all of it.
>

This CL was about implementing only IDBDatabase::createObjectStore(). I promise to fix all the plumbing but it's easier for me to do so in smaller CLs rather than all at once.
 
> 
> Close...I should probably do one more scan through though.

Thanks for the review, will send a new patch asap.

[Nate Chapin]

(In reply to comment #17)
> Nate, can you please look at the code generator part of this?

LGTM

[Andrei Popescu]

Created attachment 57882 [details]
Patch

[Jeremy Orlow]

Comment on attachment 57882 [details]
Patch

WebCore/ChangeLog:12
 +          * Android.mk:
regenerate list

WebCore/storage/IndexedDatabaseRequest.cpp:57
 +          // FIXME: make this work with workers.
ASSERT_NOT_REACHED()
WebCore/storage/IndexedDatabaseRequest.cpp:63
 +          return 0;
Didn't we agree to return an IDBRequest that'll just never fire?

WebCore/storage/IndexedDatabaseRequest.cpp:61
 +      Document* doc = static_cast<Document*>(context);
don't abbreviate 

WebKit/chromium/public/WebIDBDatabase.h:64
 +          WEBKIT_ASSERT_NOT_REACHED();
Might be slightly better to assign the WebIDBCallbacks to an OwnPtr so you don't leak and to make it obvious to whomever implements this that it's an owning relationship...but this code is going away soon anyway so I don't care much.

[Jeremy Orlow]

r=me with those changes

[Andrei Popescu]

(In reply to comment #22)
> r=me with those changes

Thanks! I'll do the changes and land it manually.

[Andrei Popescu]

Committed r60776