Safari 10 IndexedDB compatibility #5572

Open
nolanlawson opened this Issue Aug 17, 2016 · 29 comments

Projects

None yet

4 participants

@nolanlawson
Member

Starting a new issue to keep track of this.

In order to use IDB in Safari 10, we would need to:

  1. Establish that it passes our IDB test suite
  2. Establish that it's not unreasonably slower than WebSQL (e.g. for Chrome it's up to ~20% slower depending on the use case, which is acceptable).

Preliminary testing shows that Safari 10 does not pass the IDB test suite, but I haven't yet nailed down the cause. It seems intermittent. I haven't had time yet to diagnose.

Steps to repro:

git clone https://github.com/pouchdb/pouchdb --branch safari-10 --single-branch --depth 1
cd pouchdb
npm install
# install couchdb and get it running on localhost:5984, # e.g. `brew install couchdb`
npm run dev
# point Safari to http://localhost:8000/tests/integration
@nolanlawson
Member

I ran the tests about a month ago and saw some intermittent hangs, but just re-ran them and happy to say it almost reached 100%, except for what appears to be an unrelated timeout:

screenshot 2016-08-17 08 10 14

(That 0% is a bug in Mocha.)

OTOH Safari IDB does indeed appear to be extremely slow; many of the more elaborate all_docs tests take several seconds to run.

@nolanlawson
Member

This was in Safari 10.0 (12602.1.38.2) on macOS Sierra BTW.

@beidson
beidson commented Aug 18, 2016

Just for a sanity check, if you can easily bump the timeout and confirm WebKit passes 100%, that would be good to know.

@nolanlawson
Member

I keep restarting the test, but it often fails due to non-IDB-related HTTP timeouts apparently. Will try to give this a thorough pass this weekend.

@llin96
llin96 commented Sep 8, 2016

@nolanlawson Is there any plan to support IndexedDB on iOS Safari 10?

@nolanlawson
Member

I have not worked on it. I couldn't get Safari 10 to fully pass the test suite, apparently due to HTTP issues (non-IDB stuff). In general it seems to be working but also seems to be incredibly slow (~50x slower than WebSQL), so I'm not sure we'd be doing any favors to our users to upgrade.

Furthermore iOS 10 also adds WebSQL support back to WKWebView, so this gives us even more incentive to just stick with WebSQL for now. Plus we are working on a complete rewrite of our IndexedDB adapter that will support IDB in all engines ("idb-next"), so it may be worth our time to just focus on that instead.

@nolanlawson nolanlawson referenced this issue in localForage/localForage Sep 15, 2016
Closed

Safari 10 detection for indexedDB fails #604

@nolanlawson
Member
nolanlawson commented Sep 17, 2016 edited

@dfahlander reached out to me to ask about IDB in WebKit, and pointed out an open issue showing that it's possible for indexes to collide across object stores. He has a test suite that I can confirm fails in Safari 10.0 (12602.1.50.0.8) but passes in Firefox, Chrome, and Edge.

The question for me is whether this would impact PouchDB. I think we might actually be okay in this case; the IDB tests seem to pass, and furthermore I think our keys across object stores are very unlikely to collide. The only way they could collide is if e.g. the same revision ID was used as a document ID, or the same attachment hash was used as a document ID. However, it's worth checking. I'm not sure if, for instance, you create a document with an integer-like string ID, if that would collide with the by-seq object store.

@beidson Since Safari 10 is close to release, you may want to look at David's issue and test suite; it seems it could be a blocker for Dexie users at least.

@dfahlander
dfahlander commented Sep 17, 2016 edited

@nolanlawson I've added a gist that reproduces this: https://bl.ocks.org/dfahlander/a7a0608513d527742c5a09e3c95dd343. The strange behaviour occurs when opening a cursor on an index when there are multiple stores. There's no primary key collision problem, just that the index visits 'strangers' on its way and you will get faulty results from querying indexes in general, which is quite severe IMO. I'm going to update the webkit bug with this repro right now as well.

EDIT: The issue is actually triggered only when there are primary key collisions between object stores, but not in the same way as in Safari 9. In Safari 10 it will affect the result of index queries.

@nolanlawson
Member

@dfahlander Hm, I'm surprised the Pouch test suite didn't catch this then, because we have multiple object stores and also quite a few indexes. I'll look into it when I have time and see if I can reproduce with Pouch alone. Thanks!

@dfahlander
dfahlander commented Sep 19, 2016 edited

Nailed it down to that the bug is triggered when documents have same primary key at different object stores (as happens when creating the ObjectStores with options {autoIncrement: true}) and after adding some items to the stores, querying one store using an index. This might be a savior for Pouch, as nolan stated earlier, since PouchDB's keys across object stores are very unlikely to collide.

@nolanlawson
Member

Yes, it means the bug would be very rare, unless it’s possible to have collisions for IDs of different types. We have a by-sequence table that uses monodically incrementing integers, and a by-doc table that uses arbitrary strings; if a user names a document “1” and this collides with the integer 1, would that trigger the bug too?

If not, then yeah, it may be extremely rare for this bug to affect PouchDB due to our objectStore IDs rarely colliding. But I would need to re-analyze our objectStore structures and verify this.

–Nolan

On Sep 19, 2016, at 1:48 PM, David Fahlander notifications@github.com wrote:

Nailed it down to that the bug is triggered when documents have same primary key at different object stores (as happens when creating the ObjectStores with options {autoIncrement: true}). This might be a savior for Pouch, I suppose?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #5572 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AARUwqxPCavepS7ol37a6Egk_vTllS4Hks5qrvU3gaJpZM4JmjI1.

@dfahlander

Update: Bug was fixed last thursday (Thanks a lot @beidson !). This doesn't necessarily mean we know it's gonna go to production very soon of course, but lets hope so.

@nolanlawson
Member

Sweeeeet! In that case that just leaves perf that we need to analyze. :)

@nolanlawson
Member

I set up a version of PouchDB's test suite that only tests IDB (no CouchDB/xhr/fetch/WebSQL/etc.) and Safari 10.0.2 passes at 100%. 🎉 🎈 🎉 http://bl.ocks.org/nolanlawson/raw/9af8f79637b6f0d553ee121ef6fdd428/

@beidson
beidson commented Nov 21, 2016

That page is pretty subresource intensive; Do you by any chance have an easy way to make it an all-in-one?

If not, I can work on it manually.

@nolanlawson
Member

Hmm yeah it basically just needs all the test scripts to be concatenated. I
don't think it can easily be turned into a single HTML file though if
that's what you mean.

BTW it's a gist, so feel free to fork:
https://gist.github.com/nolanlawson/9af8f79637b6f0d553ee121ef6fdd428


Nolan Lawson
http://nolanlawson.com
https://github.com/nolanlawson

On Nov 21, 2016 1:57 PM, "beidson" notifications@github.com wrote:

That page is pretty subresource intensive; Do you by any chance have an
easy way to make it an all-in-one?

If not, I can work on it manually.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#5572 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARUwt_u21_BeKOoLQmW3rRIY_BLlpg7ks5rAhPXgaJpZM4JmjI1
.

@llin96
llin96 commented Nov 22, 2016

@nolanlawson It is a great news. Do you have a chance to test it on iOS Safari 10?

We used PouchDB to build an offline website with large scale of data stored in local, it works smoothly in desktop and android tablets.

As iPad has better performance and experience, now our client asked to make it works on iPad, some of the markets have bought iPads for their employees (The website is for internal use).

So, supporting Safari IDB becomes critical to our project. To have more information on this we can better evaluate the risk and set the correct explanation to our client, you mentioned you are rewriting the IDB adapter to support all engines, Could you share me the status of that and the plan to support iOS Safari?

@beidson
beidson commented Nov 22, 2016

BTW it's a gist, so feel free to fork:
https://gist.github.com/nolanlawson/9af8f79637b6f0d553ee121ef6fdd428

That'll help, thanks!

@nolanlawson
Member

See #5701 (comment) for details, but I think it may be wise to hold off on IDB-as-default for Safari 10 for the time being due to perf issues.

@beidson I extracted PouchDB's perf benchmark into a standalone test you can run to see the perf difference between IDB and WebSQL. Safari TP does indeed show some improvement:

Note that PouchDB's implementation is known to perform slightly better in Chrome WebSQL than in Chrome IDB, and for certain cases it's probably impossible for IDB to beat it (e.g. querying, due to overhead of IDB Cursors vs one big SQL statement).

@beidson
beidson commented Dec 12, 2016

Thanks for pulling those tests out - Should help fill the gaps in our perf work.

@beidson
beidson commented Dec 15, 2016

No browser gets through pull-replication-one-generation - undefined.

But Chrome and Firefox cruise through the first 7:
basic-inserts
bulk-inserts
basic-updates
basic-gets
all-docs-skip-limit
all-docs-startkey-endkey
all-docs-include-docs

If I just visit the main test url - http://bl.ocks.org/nolanlawson/raw/15d054371cfe240d3787c8256464a387/ - Safari Technology Preview will, indeed, hang on bulk-inserts.

But if you run bulk-inserts directly, it passes fine.

In fact, STP passes each of the 7 tests fine when run by themselves.

I saved a local copy of the tests to try to find out what's going on but wow is that a lot of complicated javascript built around a storage abstraction framework that effectively hides all the inner-workings of IndexedDB.

Trying to figure out why tests don't advance to the next one is an exercise in futility.

I'll try running against a debug built of WebKit with lots of logging and see if anything obvious is happening (A transaction not committing/aborting, a connection not being closed, etc etc)

@nolanlawson
Member
@beidson
beidson commented Dec 15, 2016

I understand it might not be the most reduced test case, but hopefully it's
useful for stress testing at least.

To repeat my last comment, if I start the full suite, Safari hangs on bulk-inserts, but I can run each individual test just fine.

So the problem I'm having with the complexity of the test's code base is that I can't reduce in any meaningful manner to find out why the full harness hangs.

It absolutely might be a WebKit bug, but it might also be the test harness/PouchDB making assumptions based on other engine's behavior even though WebKit is following spec.

Either way, I hope to figure it out, but "hacking on the test's code" is not a useful tactic. ;)

@nolanlawson
Member
@beidson
beidson commented Dec 15, 2016

Yeah my guess in that case is that it's because the
benchmark destroys and recreates the db between each run, so it could be
triggering a race condition related to deletion.

Definitely not this.

I'm pretty sure we don't have any WebKit-sniffing behavior...

Yah, I don't mean it explicitly sniffs for WebKit, but rather as it was being developed on Chrome/FFX assumptions were made based on their behaviors instead of the spec.

@beidson
beidson commented Dec 15, 2016

(It's running all tests for me in ToT WebKit right now, actually)

@beidson
beidson commented Dec 15, 2016

You have two suites named "views" that have different tests in them, btw ;)

@beidson
beidson commented Dec 15, 2016

Testing PouchDB version 6.1.0-prerelease

Starting suite: basics

Starting test: basic-inserts with 1 assertions and 1000 iterations... done in 4817ms
Starting test: bulk-inserts with 1 assertions and 100 iterations... done in 4540ms
Starting test: basic-updates with 1 assertions and 100 iterations... done in 17423ms
Starting test: basic-gets with 1 assertions and 10000 iterations... done in 22520ms
Starting test: all-docs-skip-limit with 1 assertions and 50 iterations... done in 107018ms
Starting test: all-docs-startkey-endkey with 1 assertions and 50 iterations... done in 2238ms
Starting test: all-docs-include-docs with 1 assertions and 100 iterations... done in 6562ms

Tests Complete!

Starting suite: views

Starting test: temp-views with 1 assertions and 1 iterations... done in 7226ms
Starting test: persisted-views with 1 assertions and 10 iterations... done in 315ms
Starting test: persisted-views-stale-ok with 1 assertions and 10 iterations... done in 206ms

Tests Complete!

Starting suite: views

Starting test: basic-attachments with 1 assertions and 1000 iterations... done in 9629ms

Tests Complete!
...

Now I get to improving the bad numbers. Thanks!

@nolanlawson
Member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment