Last Comment Bug 704464 - IndexedDB: Quota prompt not launched when creating new database
: IndexedDB: Quota prompt not launched when creating new database
Status: RESOLVED FIXED
fixed-in-bs
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on:
Blocks: idb
  Show dependency treegraph
 
Reported: 2011-11-22 06:08 PST by Jan Varga [:janv]
Modified: 2012-03-22 11:52 PDT (History)
5 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Rework quota handling to use Windows instead of Databases. (27.58 KB, patch)
2011-11-28 04:46 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
bent.mozilla: review-
Details | Diff | Review
Part 2: Make database creation hook into the quota system. (9.07 KB, patch)
2011-11-28 04:47 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
bent.mozilla: review+
Details | Diff | Review
Part 0: Add TLS wrapper so that I don't have to burn my eyes looking at NSPR calls (3.33 KB, patch)
2011-12-01 05:32 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
bent.mozilla: review-
Details | Diff | Review
Part 1: Rework quota handling to use Windows instead of Databases (27.16 KB, patch)
2011-12-01 05:33 PST, Kyle Huey [:khuey] (khuey@mozilla.com)
bent.mozilla: review+
Details | Diff | Review

Description Jan Varga [:janv] 2011-11-22 06:08:06 PST
Say the allocated space is near the 50 MB limit, for example 49.9MB. New database takes approximately 0.7 MB. Now when a new database is created the quota exceeded callback is called. However since there's no current IDBDatabase, the operation just silently fails w/o launching the quota check prompt.

This bug blocks the files in idb bug, since we now hit the 50MB limit (some new tests were added recently).
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-28 04:46:46 PST
Created attachment 577220 [details] [diff] [review]
Part 1: Rework quota handling to use Windows instead of Databases.
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-28 04:47:24 PST
Created attachment 577221 [details] [diff] [review]
Part 2: Make database creation hook into the quota system.
Comment 3 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-30 16:16:36 PST
Comment on attachment 577220 [details] [diff] [review]
Part 1: Rework quota handling to use Windows instead of Databases.

Let's stick with TLS and not use a lock... The lock you have is going to be very popular, and we really only need to lock in the unlikely case that we hit our quota limit.
Comment 4 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-01 05:32:46 PST
Created attachment 578233 [details] [diff] [review]
Part 0: Add TLS wrapper so that I don't have to burn my eyes looking at NSPR calls
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-01 05:33:39 PST
Created attachment 578235 [details] [diff] [review]
Part 1: Rework quota handling to use Windows instead of Databases

Now with less locking.
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-02 12:19:34 PST
Comment on attachment 578235 [details] [diff] [review]
Part 1: Rework quota handling to use Windows instead of Databases

Review of attachment 578235 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good!

::: dom/indexedDB/IDBFactory.cpp
@@ +402,5 @@
>  
>    nsIScriptContext* context = sgo->GetContext();
>    NS_ENSURE_TRUE(context, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
>  
> +  nsCAutoString origin;

nsCString

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +137,5 @@
>  
>  } // anonymous namespace
>  
>  IndexedDatabaseManager::IndexedDatabaseManager()
> +  : mQuotaHelperMutex("IndexedDatabaseManager.mQuotaHelperMutex")

Nit: remove indent

@@ +644,5 @@
>  }
>  
>  // static
> +bool
> +IndexedDatabaseManager::QuotaIsLifted()

Can you make a static inline in the header that calls Get() and then make this a regular method? That will avoid the mgr-> everywhere.

@@ +646,5 @@
>  // static
> +bool
> +IndexedDatabaseManager::QuotaIsLifted()
> +{
> +  IndexedDatabaseManager *mgr = IndexedDatabaseManager::Get();

Nit: * on the left

@@ +697,5 @@
> +}
> +
> +// static
> +void
> +IndexedDatabaseManager::CancelPromptsForWindow(nsPIDOMWindow* aWindow)

Static inline thing again.

@@ +701,5 @@
> +IndexedDatabaseManager::CancelPromptsForWindow(nsPIDOMWindow* aWindow)
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +
> +  IndexedDatabaseManager *mgr = IndexedDatabaseManager::Get();

* again.
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-02 12:20:12 PST
Comment on attachment 578233 [details] [diff] [review]
Part 0: Add TLS wrapper so that I don't have to burn my eyes looking at NSPR calls

Let's skip this like we discussed ;)
Comment 8 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-02 12:22:52 PST
Comment on attachment 577221 [details] [diff] [review]
Part 2: Make database creation hook into the quota system.

Review of attachment 577221 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/IndexedDatabaseManager.h
@@ +318,5 @@
>    // thread.
>    nsTArray<nsCString> mTrackedQuotaPaths;
>  };
>  
> +class AutoEnterWindow {

Nit: { on next line

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +1091,5 @@
>    }
>  
> +  NS_ASSERTION(mOpenDBRequest, "This should never be null!");
> +
> +  // XXXkhuey after gabor's stuff this assertion will be bogus.

Fix this comment (and the one in the other patch) to not be XXX and to simply say that this won't be true once we allow indexeddb outside of windows.

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