702292 - IndexedDB: deleteDatabase() doesn't update quota

Status: RESOLVED WORKSFORME

(Core :: Storage: IndexedDB, defect)

Description

[Jan Varga [:janv]]

Database files are now removed directly by calling nsIFile::Remove()
This is a problem since quota is not updated.

It can be easily fixed by using virtual files (added by files in indexedDB patch). mozIStorageVirtualFile::Delete() uses SQLite's API to delete files so the quota is correctly updated.

Comment 1

[Jan Varga [:janv]]

for aurora, mozIStorageServiceQuotaManagement::updateQuotaInformationForFile() can be used to fix the issue

Comment 2

[Jan Varga [:janv]]

to save you some time, adding this:

nsCOMPtr<mozIStorageServiceQuotaManagement> ss =
  do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID);
NS_ENSURE_TRUE(ss, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
rv = ss->UpdateQuotaInformationForFile(dbFile);
NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

to DeleteDatabaseHelper::DoDatabaseWork() crashes on windows

here's the log:
1600 INFO TEST-KNOWN-FAIL | /tests/dom/indexedDB/test/test_deleteDatabase.html | null - newVersion should be null
TEST-UNEXPECTED-FAIL | /tests/dom/indexedDB/test/test_deleteDatabase.html | Exited with code -1073741819 during test run
INFO | automation.py | Application ran for: 0:02:30.195000
INFO | automation.py | Reading PID log: c:\users\cltbld\appdata\local\temp\tmpci9gfrpidlog
PROCESS-CRASH | /tests/dom/indexedDB/test/test_deleteDatabase.html | application crashed (minidump found)
Crash dump filename: c:\users\cltbld\appdata\local\temp\tmpyvwtrq\minidumps\dd497dad-a5c5-4201-8368-77051de4681b.dmp
Operating system: Windows NT
                  6.1.7600 
CPU: x86
     GenuineIntel family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
Crash address: 0xffffffffcdcdcdcd

Thread 33 (crashed)
 0  xul.dll!`anonymous namespace'::xOpen(sqlite3_vfs *,char const *,sqlite3_file *,int,int *) [TelemetryVFS.cpp:0d614da19ca1 : 331 + 0x0]
    eip = 0x6905338e   esp = 0x11fbfc28   ebp = 0x11fbfc6c   ebx = 0x11369330
    esi = 0x0f44db10   edi = 0xcdcdcdcd   eax = 0x0f44db10   ecx = 0x00000000
    edx = 0x00000000   efl = 0x00010246
    Found by: given as instruction pointer in context
 1  xul.dll!quotaOpen [test_quota.c:0d614da19ca1 : 379 + 0xa]
    eip = 0x6903e6d5   esp = 0x11fbfc74   ebp = 0x11fbfc9c   ebx = 0x00000101
    Found by: call frame info
 2  xul.dll!sqlite3_quota_file(char const *) [test_quota.c:0d614da19ca1 : 830 + 0x14]
    eip = 0x6903ee61   esp = 0x11fbfca4   ebp = 0x11fbfcd0   ebx = 0x00000000
    Found by: call frame info
 3  xul.dll!mozilla::storage::Service::UpdateQuotaInformationForFile(nsIFile *) [mozStorageService.cpp:0d614da19ca1 : 744 + 0x13]
    eip = 0x6903f472   esp = 0x11fbfcd8   ebp = 0x11fbfcfc   ebx = 0x00000000
    Found by: call frame info
 4  xul.dll!`anonymous namespace'::DeleteDatabaseHelper::DoDatabaseWork(mozIStorageConnection *) [OpenDatabaseHelper.cpp:0d614da19ca1 : 1455 + 0x16]
    eip = 0x68c9a5d3   esp = 0x11fbfd04   ebp = 0x11fbfdf4
    Found by: call frame info
 5  xul.dll!mozilla::dom::indexedDB::AsyncConnectionHelper::Run() [AsyncConnectionHelper.cpp:0d614da19ca1 : 300 + 0x9]
    eip = 0x68c6c303   esp = 0x11fbfdfc   ebp = 0x11fbfe18
    Found by: call frame info
 6  xul.dll!nsThread::ProcessNextEvent(bool,bool *) [nsThread.cpp:0d614da19ca1 : 631 + 0xd]
    eip = 0x692f4678   esp = 0x11fbfe20   ebp = 0x11fbfe4c   ebx = 0x00000000
    Found by: call frame info
 7  xul.dll!NS_ProcessNextEvent_P(nsIThread *,bool) [nsThreadUtils.cpp:0d614da19ca1 : 245 + 0xc]
    eip = 0x692b9489   esp = 0x11fbfe54   ebp = 0x11fbfe60   ebx = 0x00000000
    Found by: call frame info
 8  xul.dll!nsThread::ThreadFunc(void *) [nsThread.cpp:0d614da19ca1 : 272 + 0x7]
    eip = 0x692f3cc1   esp = 0x11fbfe68   ebp = 0x11fbfe88

Comment 3

[(dormant account)]

I think the bug here is that telemetryvfs got unregistered, but quota vfs is still pointing to it as the real vfs.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Taras Glek (:taras) from comment #3)
> I think the bug here is that telemetryvfs got unregistered, but quota vfs is
> still pointing to it as the real vfs.

So, who is going to fix that?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Taras Glek (:taras) from comment #3)
> I think the bug here is that telemetryvfs got unregistered, but quota vfs is
> still pointing to it as the real vfs.

Chatting on irc we decided that this theory is most likely incorrect. VFS unregistration only happens at shutdown, and this crash happens during normal runtime.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think this code is just broken.

At http://hg.mozilla.org/mozilla-central/annotate/854aabf544d4/db/sqlite3/src/test_quota.c#l822 we allocate a sqlite3_file.  We don't do anything to this memory so in a debug Windows build it's filled with 0xcdcdcdcd.

Then we call open on this uninitialized file at http://hg.mozilla.org/mozilla-central/annotate/854aabf544d4/db/sqlite3/src/test_quota.c#l829

That passes the file to the quotaOpen code which calls into the VFS at http://hg.mozilla.org/mozilla-central/annotate/854aabf544d4/db/sqlite3/src/test_quota.c#l379  The telemetry VFS decides it doesn't care about this operation and passes through to the original VFS at http://hg.mozilla.org/mozilla-central/annotate/accfde65e3c7/storage/src/TelemetryVFS.cpp#l326.  The original VFS decides it can't open the file (because the file doesn't exist on the disk and returns.)  Then the telemetry VFS tries to assign its own stuff to the file's VFS (or something) and fails trying to store to 0xcdcdcdcd at http://hg.mozilla.org/mozilla-central/annotate/accfde65e3c7/storage/src/TelemetryVFS.cpp#l331.

So, my question is, how does UpdateQutoaInformationForFile (sic) work at all?  I notice that we never hit it in the test suite ...

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok, a later part of sqlite3.c's winOpen sets the file structure values to the appropriate values, so this only affects files where sqlite returns an error code for opening them.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Pretty easy fix.

We'll want this on Aurora (and maybe the TelemetryVFS fix on Beta, even).

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 577322 [details] [diff] [review]
Patch

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

r=me with comments that we discussed on irc addressed.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

For posterity, bent's pastebin review:

    dom/indexedDB/OpenDatabaseHelper.cpp
    nsCOMPtr<mozIStorageServiceQuotaManagement> ss =
    do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID);
    NS_ENSURE_TRUE(ss, NS_OK); // We succeeded deleting, at least.
     
    rv = ss->UpdateQutoaInformationForFile(dbFile);
           
    You can move this inside the |if (exists)| block, right? If the file doesn't exist then presumably the quota doesn't take it into account?
    storage/src/TelemetryVFS.cpp
    break;
    }
    p->histograms = h;
    rc = orig_vfs->xOpen(orig_vfs, zName, p->pReal, flags, pOutFlags);
    if( rc != SQLITE_OK)
           
    Nit: Style is all messed up in this file, I see:
    if( cond )
    and:
    if(cond)
    and:
    if (cond)
    Just pick one instead of this funny hybrid you've got here :)

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Review comments addressed.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 577669 [details] [diff] [review]
Patch

This would be nice to have in Aurora (it fixes a bug in a feature introduced there), but I'm not going to lose any sleep if it gets approval-.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 577669 [details] [diff] [review]
Patch

[Triage Comment]
Please re-nominate once this has landed on m-c and has had a chance to bake for a couple of days.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Kyle, is this ready to land? I think I was seeing some deleteDatabase related issue the other day and want to make sure this wasn't it.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No, files in IDB bitrotted this and I haven't had a chance to fix it up yet.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This mostly got fixed, except for the part where deleteDatabase got broken entirely, so I'm going to open a new bug.