Bug 36770 - More IndexedDB work
: More IndexedDB work
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-03-29 12:18 PST by
Modified: 2011-05-13 12:30 PST (History)


Attachments
Patch (32.94 KB, patch)
2010-03-29 12:26 PST, Jeremy Orlow
fishd: review+
fishd: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-03-29 12:18:26 PST
More IndexedDB work
------- Comment #1 From 2010-03-29 12:26:38 PST -------
Created an attachment (id=51946) [details]
Patch
------- Comment #2 From 2010-03-29 12:30:45 PST -------
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 From 2010-03-29 23:13:19 PST -------
(From update of attachment 51946 [details])
> +++ 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.
------- Comment #4 From 2010-03-30 04:36:30 PST -------
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 From 2010-03-30 04:59:16 PST -------
Committed r56777: <http://trac.webkit.org/changeset/56777>
------- Comment #6 From 2011-05-13 12:30:37 PST -------
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.