IndexedDB: Quota prompt not launched when creating new database

RESOLVED FIXED

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: janv, Assigned: khuey)

Tracking

unspecified
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-bs)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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).
Assignee: nobody → khuey
Blocks: 687337
Created attachment 577220 [details] [diff] [review]
Part 1: Rework quota handling to use Windows instead of Databases.
Attachment #577220 - Flags: review?(bent.mozilla)
Created attachment 577221 [details] [diff] [review]
Part 2: Make database creation hook into the quota system.
Attachment #577221 - Flags: review?(bent.mozilla)
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.
Attachment #577220 - Flags: review?(bent.mozilla) → review-
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
Attachment #578233 - Flags: review?(bent.mozilla)
Created attachment 578235 [details] [diff] [review]
Part 1: Rework quota handling to use Windows instead of Databases

Now with less locking.
Attachment #577220 - Attachment is obsolete: true
Attachment #578235 - Flags: review?(bent.mozilla)
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.
Attachment #578235 - Flags: review?(bent.mozilla) → review+
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 ;)
Attachment #578233 - Flags: review?(bent.mozilla) → review-
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.
Attachment #577221 - Flags: review?(bent.mozilla) → review+
Attachment #578233 - Attachment is obsolete: true
https://hg.mozilla.org/projects/build-system/rev/ffdf85abb789
https://hg.mozilla.org/projects/build-system/rev/3838d56a1eaf
Flags: in-testsuite+
Whiteboard: fixed-in-bs
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/ffdf85abb789
https://hg.mozilla.org/mozilla-central/rev/3838d56a1eaf
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla11 → ---
You need to log in before you can comment on or make changes to this bug.