767193 - FileHandles do not get GCed/removed when overwritten in indexeddb

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[(dormant account)]

Attachment: testcase(on first run creates files, on second run reads)

See the for loop in testcase. I create a bunch of filehandles and store them in indexeddb under the same key. 99/100 files end up unreferenced and not cleaned up  across restarts in the indexeddb directory. Only one of those files can still be retrieved via indexeddb api.

If I do not store those files in indexeddb, they get cleaned up as expected.

Comment 1

[Jan Varga [:janv]]

Yeah, I remember this used to work correctly.
It seems the bug was introduced by patch for bug 701772.
"INSERT OR REPLACE" doesn't fire the update trigger, only the insert one

http://code.google.com/p/android/issues/detail?id=3302
http://sqlite.1065341.n5.nabble.com/Delete-Trigger-on-INSERT-OR-REPLACE-td22403.html
http://www.sqlite.org/lang_conflict.html

I enabled recursive triggers locally (so the delete trigger fires too) and it fixes the bug.

Comment 2

[Jan Varga [:janv]]

Attachment: possible fix

Comment 3

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

Comment on attachment 636058 [details] [diff] [review]
possible fix

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

Any way to test this? We don't want to let this break again.

::: dom/indexedDB/IDBFactory.cpp
@@ +190,5 @@
>    ));
>    NS_ENSURE_SUCCESS(rv, nsnull);
>  
> +  rv = connection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "PRAGMA recursive_triggers = ON;"

Hm, can we just add this to the other statement above? We call this a lot so reducing the number of times we make calls into SQLite is probably good.

Also, please add a comment saying why we need this.

Comment 4

[Jan Varga [:janv]]

Attachment: fix

Comment 5

[Jan Varga [:janv]]

https://hg.mozilla.org/mozilla-central/rev/5cdbeae14405

Comment 6

[Jan Varga [:janv]]

btw, you can replace this code:

var lockedFile = file.getFile().lockedFile
lockedFile.getMetadata().onsuccess = function (event) {
  var metadata = event.target.result
  var request = lockedFile.readAsText(metadata.size)
  request.onsuccess = function (event) {
    log("victory " + metadata.size + event.target.result)
  }
}

with just:
var lockedFile = file.open()
lockedFile.getMetadata().onsuccess = function (event) {
  var metadata = event.target.result
  var request = lockedFile.readAsText(metadata.size)
  request.onsuccess = function (event) {
    log("victory " + metadata.size + event.target.result)
  }
}

so file.getFile() creates a read-only lock internally too, but .getFile() is rather useful only for passing file objects (snapshots) to other APIs, like FileReader or XHR

Comment 7

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

Jan: Hmm.. I think we should make getFile() simply return a DOMRequest. I didn't realize that you can place further request on the lockedFile used for getFile().

Comment 8

[Jan Varga [:janv]]

(In reply to Jonas Sicking (:sicking) from comment #7)
> Jan: Hmm.. I think we should make getFile() simply return a DOMRequest. I
> didn't realize that you can place further request on the lockedFile used for
> getFile().

yeah, the request has to be a FileRequest (holding the lockedFile) internally
but it will not expose the nsIDOMFileRequest interface

I'll try to use the NS_INTERFACE_MAP_ENTRY_CONDITIONAL macro for this

Comment 9

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

Sounds great

Comment 10

[Jan Varga [:janv]]

filed bug 771498