Bug 36770 – More IndexedDB work

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

[Jeremy Orlow]

More IndexedDB work

[Jeremy Orlow]

Created attachment 51946 [details]
Patch

[Jeremy Orlow]

Darin, if you have a few seconds...

Btw, I think this will require a re-baselining of our window enumeration layout test, but I haven't had a chance to run it yet.  That said, I think the rest of this patch is pretty much in order.

[Darin Fisher (:fishd, Google)]

Comment on attachment 51946 [details]
Patch

> +++ b/WebCore/page/PageGroup.cpp
...
> +#if ENABLE(INDEXED_DATABASE)
> +IndexedDatabase* PageGroup::indexedDatabase()
> +{
> +    // Do NOT add page settings based access controls here (like in LocalStorage above).
> +    // ChromeClient might be a good place.  Doing a check in DOMWindow::indexedDB (so this
> +    // only gets called once we've passed a check) might also be good.  But the way it
> +    // was done originally was a mistake.

This comment could use some tidying.  It might help to explain
why adding settings based access controls here would be bad.
I think you could probably leave it at that.  The last sentence
is confusing since it seems to refer to some older implementation
of IndexedDB.  Comments like that don't stand the test of time
very well.


> +++ b/WebCore/storage/IndexedDatabaseRequest.h
...
>  class IndexedDatabaseRequest : public RefCounted<IndexedDatabaseRequest> {
>  public:
> +    static PassRefPtr<IndexedDatabaseRequest> create(Page* page)
>      {
> +        return adoptRef(new IndexedDatabaseRequest(page));
>      }
>      ~IndexedDatabaseRequest();
>  
>      void open(const String& name, const String& description, bool modifyDatabase, ExceptionCode&);
>  
>  private:
> +    IndexedDatabaseRequest(Page* page);

nit: do not name the param


>  
> +    PassRefPtr<IndexedDatabase> m_indexedDatabase;
> +    Page* m_page;
>  };

Hmm... you've got a reference counted class holding a non-owning pointer
to Page.  That sounds like a recipe for trouble.  I would expect to at
least see a public method that ~Page() should call to clear m_page.


> +++ b/WebKit/chromium/src/WebIndexedDatabaseImpl.cpp
...
> +#include "config.h"
> +#include "WebIndexedDatabaseImpl.h"
> +
> +
> +#include "WebIDBDatabaseError.h"

nit: only 1 new line between those includes.


r=me w/ these issues addressed.

[Jeremy Orlow]

Fixed up the comment, changed it to take a Frame and have a disconnectFrame function (like many others), and fix the extra new line.

[Jeremy Orlow]

Committed r56777: <http://trac.webkit.org/changeset/56777>

[Adam Barth]

This patch was landed with some additional security FIXMEs:

http://trac.webkit.org/changeset/56777/trunk/WebCore/page/DOMWindow.cpp

The fix for that issue is in https://bugs.webkit.org/show_bug.cgi?id=60785.  In the future, please consider filing a security bug or dropping me an email when you add security FIXMEs.  Usually they're easy to fix, but losing track of them can be painful later.