Bug 137154

Summary:

IndexedDB is deleting data when a PK is shared amongst two objectStores

Product:

WebKit

Reporter:

Raymond Camden <raymondcamden>

Component:

DOM

Assignee:

Brady Eidson <beidson>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alecflett, beidson, buildbot, commit-queue, eoconnor, jsbell, nolan, richardconnamacher, rniwa, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch v1 for EWS	none
Patch v2 - New test	jer.noble: review+
Patch for landing.	none

Raymond Camden

Reported 2014-09-26 13:16:51 PDT

Imagine you have one indexeddb instance and two objectstores: people and notes If you add data to people with an assigned PK of 1, and then do the same to note, the data in people is erased. If you switch to using auto number PKs then the same thing happens (since they both end up having the same ID). Primary keys are supposed to be unique per objectStore, not the entire database. Here is a blog entry showing examples of this issue: http://www.raymondcamden.com/2014/9/25/IndexedDB-on-iOS-8--Broken-Bad#cFF30B25B-F01B-05AB-E8041ED50386A176

Attachments
Patch v1 for EWS (9.39 KB, patch) 2014-10-30 09:36 PDT, Brady Eidson	no flags	Details Formatted Diff Diff
Patch v2 - New test (13.76 KB, patch) 2014-10-30 12:27 PDT, Brady Eidson	jer.noble: review+	Details Formatted Diff Diff
Patch for landing. (14.01 KB, patch) 2014-10-30 13:17 PDT, Brady Eidson	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2014-09-26 17:58:26 PDT

<rdar://problem/18476324>

Brady Eidson

Comment 2 2014-09-27 12:06:59 PDT

Actually <rdar://problem/18479306>

Brady Eidson

Comment 3 2014-10-30 09:32:27 PDT

The problem is the schema: "CREATE TABLE Records (objectStoreID INTEGER NOT NULL ON CONFLICT FAIL, key TEXT COLLATE IDBKEY NOT NULL ON CONFLICT FAIL UNIQUE ON CONFLICT REPLACE, value NOT NULL ON CONFLICT FAIL)")); Which needs to be: "CREATE TABLE Records (objectStoreID INTEGER NOT NULL ON CONFLICT FAIL, key TEXT COLLATE IDBKEY NOT NULL ON CONFLICT FAIL, value NOT NULL ON CONFLICT FAIL)")); With the addition of an index to make objectstore/key pairs unique: "CREATE UNIQUE INDEX IF NOT EXISTS RecordsIndex ON Records (objectStoreID, key);" That in itself is straightforward. Unfortunately since the bad schema is out there, we also need a migration path. That was a little weird to get right, but I have now gotten it right. Patch forthcoming.

Brady Eidson

Comment 4 2014-10-30 09:36:40 PDT

Created attachment 240678 [details] Patch v1 for EWS I'm running tests locally to see which have been fixed, but I also want the bot's opinion (because my machine never agrees with them anyways)

Brady Eidson

Comment 5 2014-10-30 12:27:38 PDT

Created attachment 240686 [details] Patch v2 - New test

Jer Noble

Comment 6 2014-10-30 12:49:50 PDT

Comment on attachment 240686 [details] Patch v2 - New test View in context: https://bugs.webkit.org/attachment.cgi?id=240686&action=review r=me, with test nits. > LayoutTests/storage/indexeddb/primary-key-unique-to-objectstore.html:3 > +<body> > +<div id="logger"></div> > +<script> What, no <!DOCTYPE>? > LayoutTests/storage/indexeddb/primary-key-unique-to-objectstore.html:11 > +function log(msg) { > + document.getElementById("logger").innerHTML += msg + "<br>"; > +} I'm pretty sure we've already got mechanisms for this laying around in LayoutTest .js files somewhere. No biggie though. > LayoutTests/storage/indexeddb/primary-key-unique-to-objectstore.html:19 > + thisDB.createObjectStore("people", {keyPath:"id"}); nit: spaces-not-tabs (here and elsewhere) > LayoutTests/storage/indexeddb/primary-key-unique-to-objectstore.html:34 > +openRequest.onerror = function(e) { > + log("openRequest error - " + e); > +} Do you want to notifyDone() here too? Just to avoid a test timeout in the case of an unrecoverable error. > LayoutTests/storage/indexeddb/primary-key-unique-to-objectstore.html:43 > + var transaction = db.transaction(["people"],"readwrite"); nit: space-after-comma > LayoutTests/storage/indexeddb/primary-key-unique-to-objectstore.html:51 > + name:"Person", > + id:sharedID nit: space-after-colon > LayoutTests/storage/indexeddb/primary-key-unique-to-objectstore.html:69 > + note:"Note", > + uid:sharedID Ditto. > LayoutTests/storage/indexeddb/primary-key-unique-to-objectstore.html:98 > + var transaction = db.transaction(["people"],"readwrite"); nit: space-after-comma > LayoutTests/storage/indexeddb/primary-key-unique-to-objectstore.html:107 > + log("Error getting person - ", e); notifyDone()? > LayoutTests/storage/indexeddb/primary-key-unique-to-objectstore.html:119 > + var transaction2 = db.transaction(["notes"],"readwrite"); > + nit: space-after-comma > LayoutTests/storage/indexeddb/primary-key-unique-to-objectstore.html:134 > + for (n in e.target.result) { > + log(n); > + } Is result an array? Could you do: "e.target.result.forEach(log);"? > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:64 > +static const String v2RecordsTableSchema(const String& tableName) ... > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:74 > +static const String& v2RecordsTableSchema() These two methods having the same name (with different parameters) is a little weird. I mean, this second method is literally for the table named "Records", but the first isn't. I can't think of anything better for the first name though. v2RecordsTableLikeSchemaWithName()?

Brady Eidson

Comment 7 2014-10-30 13:17:46 PDT

Created attachment 240689 [details] Patch for landing.

WebKit Commit Bot

Comment 8 2014-10-30 14:00:55 PDT

Comment on attachment 240689 [details] Patch for landing. Clearing flags on attachment: 240689 Committed r175378: <http://trac.webkit.org/changeset/175378>

WebKit Commit Bot

Comment 9 2014-10-30 14:00:58 PDT

All reviewed patches have been landed. Closing bug.

Raymond Camden

Comment 10 2015-03-10 03:27:21 PDT

For folks who view this bug, note that it is still not released as far as I know (tested iOS 8.2).

Brady Eidson

Comment 11 2015-03-11 12:07:08 PDT

(In reply to comment #10) > For folks who view this bug, note that it is still not released as far as I > know (tested iOS 8.2). This bug tracker is about the WebKit open source project. It would be good feedback to have if the bug still reproduces in WebKit nightlies or trunk builds.

Raymond Camden

Comment 12 2015-03-11 13:10:13 PDT

I'm sorry - it was pointed out to me later. I didn't mean to clutter the issue.

Nolan Lawson

Comment 13 2015-06-05 08:23:19 PDT

I've written a live test case: http://bl.ocks.org/nolanlawson/0ee229ca3bf4b650286e I'm still seeing this issue in desktop Safari 8.0.7 as well as Safari on iOS 8.3. The reported user agent string says "AppleWebKit/600.7.5", so I'm surprised it's still happening.

Brady Eidson

Comment 14 2015-06-05 08:36:32 PDT

(In reply to comment #13) > I've written a live test case: > http://bl.ocks.org/nolanlawson/0ee229ca3bf4b650286e > > I'm still seeing this issue in desktop Safari 8.0.7 as well as Safari on iOS > 8.3. The reported user agent string says "AppleWebKit/600.7.5", so I'm > surprised it's still happening. Hi Nolan, When you're referring to Safari 8.0.7 and iOS 8.3, you're referring to products that Apple releases. Their configuration and scheduling are outside the purview of the WebKit open source project. What would be useful info would be whether or not this still reproduces for you using a WebKit nightly. Have you tried that?

Nolan Lawson

Comment 15 2015-06-05 09:29:39 PDT

Hi Brady, I just tried Webkit r185238, and my test case passes. :) Thanks a bunch! Looking forward to when this lands in an Apple release.

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