692652 - IndexedDB: Index updating is broken

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Comment 1

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

Ben know's the specifics here.

Comment 2

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

Attachment: 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.

Comment 3

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

Attachment: Patch, v2

This is better!

Comment 4

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

Attachment: Patch, v3

Third time's the charm!

Comment 5

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

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.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/91e5b6428bf1

Comment 7

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

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.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/91e5b6428bf1

Comment 9

[LegNeato]

(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.

Comment 10

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

Well, we really don't want an ESR based on the old IndexedDB setVersion syntax ...

Yes, I did just open that can of worms.

Comment 11

[LegNeato]

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!

Comment 12

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

https://hg.mozilla.org/releases/mozilla-beta/rev/7b86115a3601

Comment 13

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

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

Comment 14

[u279076]

Is there something QA can do to verify this fix?

Comment 15

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

I think our automated tests are covering it. If you want to help writing more automated tests though that'd be awesome ;-)