Bug 36770

Summary: More IndexedDB work
Product: WebKit Reporter: Jeremy Orlow <jorlow>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: abarth, fishd
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Description Flags
Patch fishd: review+, fishd: commit-queue-

Description Jeremy Orlow 2010-03-29 12:18:26 PDT
More IndexedDB work
Comment 1 Jeremy Orlow 2010-03-29 12:26:38 PDT
Created attachment 51946 [details]
Comment 2 Jeremy Orlow 2010-03-29 12:30:45 PDT
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.
Comment 3 Darin Fisher (:fishd, Google) 2010-03-29 23:13:19 PDT
Comment on attachment 51946 [details]

> +++ b/WebCore/page/PageGroup.cpp
> +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.
Comment 4 Jeremy Orlow 2010-03-30 04:36:30 PDT
Fixed up the comment, changed it to take a Frame and have a disconnectFrame function (like many others), and fix the extra new line.
Comment 5 Jeremy Orlow 2010-03-30 04:59:16 PDT
Committed r56777: <http://trac.webkit.org/changeset/56777>
Comment 6 Adam Barth 2011-05-13 12:30:37 PDT
This patch was landed with some additional security FIXMEs:


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.