Last Comment Bug 692652 - IndexedDB: Index updating is broken
: IndexedDB: Index updating is broken
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
Depends on:
Blocks: idb
  Show dependency treegraph
 
Reported: 2011-10-06 16:43 PDT by Jonas Sicking (:sicking) PTO Until July 5th
Modified: 2012-03-22 11:51 PDT (History)
7 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Patch, v1 (9.18 KB, patch)
2011-11-18 22:16 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v2 (15.53 KB, patch)
2011-11-21 15:02 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Patch, v3 (20.51 KB, patch)
2011-11-21 16:11 PST, Ben Turner (not reading bugmail, use the needinfo flag!)
jonas: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Review

Description Jonas Sicking (:sicking) PTO Until July 5th 2011-10-06 16:43:42 PDT

    
Comment 1 Jonas Sicking (:sicking) PTO Until July 5th 2011-10-12 15:44:47 PDT
Ben know's the specifics here.
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-18 22:16:27 PST
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.
Comment 3 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-21 15:02:51 PST
Created attachment 575986 [details] [diff] [review]
Patch, v2

This is better!
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-21 16:11:22 PST
Created attachment 576016 [details] [diff] [review]
Patch, v3

Third time's the charm!
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2011-11-21 16:38:45 PST
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!) 2011-11-21 20:21:41 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/91e5b6428bf1
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2011-11-21 21:19:19 PST
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] 2011-11-22 09:08:37 PST
https://hg.mozilla.org/mozilla-central/rev/91e5b6428bf1
Comment 9 christian 2011-11-22 09:55:48 PST
(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 [:khuey] (khuey@mozilla.com) 2011-11-22 10:01:21 PST
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 christian 2011-11-22 17:49:05 PST
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!) 2011-11-23 00:52:03 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/7b86115a3601
Comment 13 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-11-23 09:49:32 PST
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-01 14:41:38 PST
Is there something QA can do to verify this fix?
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2011-12-01 15:06:45 PST
I think our automated tests are covering it. If you want to help writing more automated tests though that'd be awesome ;-)

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