The default bug view has changed. See this FAQ.

FileHandles do not get GCed/removed when overwritten in indexeddb

RESOLVED FIXED in mozilla16

Status

()

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

People

(Reporter: (dormant account), Assigned: janv)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

3.38 KB, text/html
Details
fix
4.15 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 636058 [details] [diff] [review]
possible fix
Assignee: nobody → Jan.Varga
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Attachment #636058 - Flags: review?(bent.mozilla)
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.
(Assignee)

Updated

5 years ago
Attachment #636058 - Flags: review?(bent.mozilla)
(Assignee)

Comment 4

5 years ago
Created attachment 636588 [details] [diff] [review]
fix
Attachment #636058 - Attachment is obsolete: true
Attachment #636588 - Flags: review?(bent.mozilla)
Attachment #636588 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/5cdbeae14405
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
(Assignee)

Comment 6

5 years ago
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
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().
(Assignee)

Comment 8

5 years ago
(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
Sounds great
(Assignee)

Comment 10

5 years ago
filed bug 771498
You need to log in before you can comment on or make changes to this bug.