The default bug view has changed. See this FAQ.

IndexedDB: Index updating is broken

RESOLVED FIXED

Status

()

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

People

(Reporter: sicking, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

unspecified
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox9 fixed, firefox10 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
Ben know's the specifics here.
Created attachment 575623 [details] [diff] [review]
Patch, v1

Here we go.

The basic issue is that we had a table like this:

  index_id | value | object_data_key | object_data_id
  ---------+-------+-----------------+---------------
      1    |   2   |      "foo"      |       10
      1    |   3   |      "bar"      |       14

Our old query said that calling Put or Update should INSERT OR REPLACE any entry that caused a constraint failure (like, changing the 'value' from 2 to 3 on object_data_id = 10). The old query assumed that the row with object_data_id = 14 would be deleted to satisfy the constraint and that the row with object_data_id = 10 would be updated. Instead, SQLite simply removes the second row and appends a new row like so:

  index_id | value | object_data_key | object_data_id
  ---------+-------+-----------------+---------------
      1    |   2   |      "foo"      |       10
      1    |   3   |      "foo"      |       10

The attached patch fixes things.
Attachment #575623 - Flags: review?(jonas)
Created attachment 575986 [details] [diff] [review]
Patch, v2

This is better!
Attachment #575623 - Attachment is obsolete: true
Attachment #575623 - Flags: review?(jonas)
Attachment #575986 - Flags: review?(jonas)
Created attachment 576016 [details] [diff] [review]
Patch, v3

Third time's the charm!
Attachment #575986 - Attachment is obsolete: true
Attachment #575986 - Flags: review?(jonas)
Attachment #576016 - Flags: review?(jonas)
Comment on attachment 576016 [details] [diff] [review]
Patch, v3

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

r=me

We really should try to get this into FF9 given that it's a data-corruption issue which could easily lead to data-loss in the application layer.
Attachment #576016 - Flags: review?(jonas) → review+
Summary: IndexedDB: fix bug with index updating → IndexedDB: Index updating is broken
https://hg.mozilla.org/integration/mozilla-inbound/rev/91e5b6428bf1
Comment on attachment 576016 [details] [diff] [review]
Patch, v3

Requesting approval on aurora and beta.

The result of this bug is corruption of data in indexedDB when certain (very common) operations are performed. This corruption can lead to unknown issues in the web application, including dataloss.

The changes are non-trivial but pretty simple. The hardest part here was to make sure that we fix all cases that are commonly broke. So the main risk isn't regression but rather that the fix isn't complete.

In the worst case scenario this only affects indexedDB, so basically no risk of regressing any major sites yet.

It would be really awesome to fix this bug for the ESR release as we're likely to see more indexedDB usage over the coming year.
Attachment #576016 - Flags: approval-mozilla-beta?
Attachment #576016 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/91e5b6428bf1
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11

Comment 9

5 years ago
(In reply to Jonas Sicking (:sicking) from comment #7)
> Comment on attachment 576016 [details] [diff] [review] [diff] [details] [review]
> Patch, v3
> 
> It would be really awesome to fix this bug for the ESR release as we're
> likely to see more indexedDB usage over the coming year.

And a major reason why I didn't want an ESR comes to the forefront before the ESR is even out! :-)

Let's take this on 9 and 10.

Updated

5 years ago
Attachment #576016 - Flags: approval-mozilla-beta?
Attachment #576016 - Flags: approval-mozilla-beta+
Attachment #576016 - Flags: approval-mozilla-aurora?
Attachment #576016 - Flags: approval-mozilla-aurora+
Well, we really don't want an ESR based on the old IndexedDB setVersion syntax ...

Yes, I did just open that can of worms.
Flags: in-testsuite+

Comment 11

5 years ago
I landed this on aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/80ae8a77e6ac

But it doesn't apply cleanly on beta. Please port it there. Thanks!
status-firefox10: --- → fixed
status-firefox9: --- → affected
https://hg.mozilla.org/releases/mozilla-beta/rev/7b86115a3601
status-firefox9: affected → fixed
Also needed to fix the new tests, they were using features that don't exist on beta.

https://hg.mozilla.org/releases/mozilla-beta/rev/6abf85b9b750
Is there something QA can do to verify this fix?
Whiteboard: [qa?]
I think our automated tests are covering it. If you want to help writing more automated tests though that'd be awesome ;-)
Whiteboard: [qa?] → [qa-]
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla11 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.