Bug 137154

Summary: IndexedDB is deleting data when a PK is shared amongst two objectStores
Product: WebKit Reporter: Raymond Camden <raymondcamden>
Component: DOMAssignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, buildbot, commit-queue, eoconnor, jsbell, nolan, richardconnamacher, rniwa, webkit-bug-importer
Priority: P2 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

Description Raymond Camden 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
Comment 1 Radar WebKit Bug Importer 2014-09-26 17:58:26 PDT 
<rdar://problem/18476324>
Comment 2 Brady Eidson 2014-09-27 12:06:59 PDT 
Actually <rdar://problem/18479306>
Comment 3 Brady Eidson 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.
Comment 4 Brady Eidson 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)
Comment 5 Brady Eidson 2014-10-30 12:27:38 PDT 
Created attachment 240686 [details]
Patch v2 - New test
Comment 6 Jer Noble 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()?
Comment 7 Brady Eidson 2014-10-30 13:17:46 PDT 
Created attachment 240689 [details]
Patch for landing.
Comment 8 WebKit Commit Bot 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>
Comment 9 WebKit Commit Bot 2014-10-30 14:00:58 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 10 Raymond Camden 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).
Comment 11 Brady Eidson 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.
Comment 12 Raymond Camden 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.
Comment 13 Nolan Lawson 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.
Comment 14 Brady Eidson 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?
Comment 15 Nolan Lawson 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.