Last Comment Bug 767193 - FileHandles do not get GCed/removed when overwritten in indexeddb
: FileHandles do not get GCed/removed when overwritten in indexeddb
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jan Varga [:janv]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-21 16:22 PDT by (dormant account)
Modified: 2012-07-06 06:36 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase(on first run creates files, on second run reads) (3.38 KB, text/html)
2012-06-21 16:22 PDT, (dormant account)
no flags Details
possible fix (850 bytes, patch)
2012-06-23 03:26 PDT, Jan Varga [:janv]
no flags Details | Diff | Review
fix (4.15 KB, patch)
2012-06-25 20:59 PDT, Jan Varga [:janv]
bent.mozilla: review+
Details | Diff | Review

Description (dormant account) 2012-06-21 16:22:54 PDT
Created attachment 635521 [details]
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] 2012-06-22 22:19:06 PDT
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] 2012-06-23 03:26:50 PDT
Created attachment 636058 [details] [diff] [review]
possible fix
Comment 3 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-25 02:15:24 PDT
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] 2012-06-25 20:59:03 PDT
Created attachment 636588 [details] [diff] [review]
fix
Comment 5 Jan Varga [:janv] 2012-06-26 22:02:37 PDT
https://hg.mozilla.org/mozilla-central/rev/5cdbeae14405
Comment 6 Jan Varga [:janv] 2012-07-03 01:53:41 PDT
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) 2012-07-05 17:18:16 PDT
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] 2012-07-05 22:54:06 PDT
(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) 2012-07-06 00:17:31 PDT
Sounds great
Comment 10 Jan Varga [:janv] 2012-07-06 06:36:48 PDT
filed bug 771498

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