localforage causing race conditions with IndexedDB and WebSQL

[jamesmfriedman]

Interesting bug that took me forever to track down.

Basically, unless you explicitly set driver in localforage.config, Chrome and Safari are indeterministic about which driver they will use, resulting in what "seems" like data loss. This happens at random.

So basically, I have 2 databases, and I'll be connected to one of them through localforage on any given reload.

UPDATE:

IndexedDB detection flat out fails half the time on chrome. I've tried debugging this for the past hour and haven't made any progress. Setting the driver explicitly to indexeddb leads to "storage not found" errors.

[jamesmfriedman]

I found the root cause of the issue in the function that detects if isIndexDb is valid.

The issue here is that browsers can be emulating behaviors with polyfills. My specific issue is even more cryptic: My last pass extension in Chrome overrides window.fetch so it is no longer a native function.

function isIndexedDBValid() {
    try {
        // Initialize IndexedDB; fall back to vendor-prefixed versions
        // if needed.
        console.log('1');
        if (!idb) {
            return false;
        }
        // We mimic PouchDB here;
        //
        // We test for openDatabase because IE Mobile identifies itself
        // as Safari. Oh the lulz...
        var isSafari = typeof openDatabase !== 'undefined' && /(Safari|iPhone|iPad|iPod)/.test(navigator.userAgent) && !/Chrome/.test(navigator.userAgent) && !/BlackBerry/.test(navigator.platform);
        // THIS IS OVER PROTECTIVE. JUST LET A WINDOW.FETCH FUNCTION THROUGH
        var hasFetch = typeof fetch === 'function' && fetch.toString().indexOf('[native code') !== -1;

        // Safari <10.1 does not meet our requirements for IDB support (#5572)
        // since Safari 10.1 shipped with fetch, we can use that to detect it
        return (!isSafari || hasFetch) && typeof indexedDB !== 'undefined' &&
        // some outdated implementations of IDB that appear on Samsung
        // and HTC Android devices <4.4 are missing IDBKeyRange
        // See: https://github.com/mozilla/localForage/issues/128
        // See: https://github.com/mozilla/localForage/issues/272
        typeof IDBKeyRange !== 'undefined';
    } catch (e) {
        return false;
    }
}

[jamesmfriedman]

I'll gladly submit a PR for this, would like a little guidance though.

[tofumatt]

Ah, interesting! Thanks for investigating this one 👍

We should try to mitigate this bug but it feels like more of a bug in LastPass. My gut feeling is they shouldn't be overriding that native method 🤷‍♂️. The whole point of our check for native code was that we wanted to know what the browser's native capability was so we could, effectively, UA sniff. I guess we're both doing bad things here 😆

LastPass is definitely big enough for this to be worth fixing though, and maybe even with a specialised patch? My first instinct is to just check for Safari's version number here instead of our window.fetch feature check, but I feel like we used window.fetch instead of a version check for a reason. Maybe not though, as that check is specific to Safari anyway.

If you could replace the hasFetch with an isSafari10_1OrGreater type of check I think that would be fine by me. It might require changing some tests–and maybe adding a few with known Safari UA strings and validating the right thing happens based on versions. I worry about UA checking introducing a lot of extra code into the package though.

If it's not something we can do reliably and without a lot of bloat, simply checking for LastPass (does it leave any markers of it running in the global scope?) and running the check differently could work. Maybe there's another Safari 10.1 feature check available?

[jamesmfriedman]

I guess the point is, you can fully polyfill both indexeddb and fetch.

There are so many random storage mechanisms built into older browsers that an indexedDb polyfill becomes useful to take advantage of them. And the same goes for fetch :).

I've already filed a bug with LastPass, but I could easily see this behavior happening in other situations. with polyfills. My only suggestion for this lib would be to do a different check (as you're suggesting) or simply drop the "native code" part of the qualification.

[jamesmfriedman]

Also as an addendum, if window.indexedDb is available, why do you care about fetch at all?

[tofumatt]

I guess the point is, you can fully polyfill both indexeddb and fetch.

If IndexedDB were polyfilled it would definitely cause errors with localForage. localForage assumes a native and pretty robust IndexedDB that is able to handle a lot of JS primitives that a polyfilled library would–likely–not support. Just guessing though.

The context for why we do this is actually because some IndexedDB implementations (like Safari's until 10.1) aren't good enough for us to rely on, so we use WebSQL instead. I just realised that the comment that explains that has an issue reference for PouchDB, so I'll edit that comment. Here's the issue though: https://github.com/pouchdb/pouchdb/issues/5572

window.fetch, in this case, is just an easy way to check for Safari's version. If there's another one that's less likely to be stomped on by an extension I'm okay with that. 🤷‍♂️

But we do need the check because we can't have older, buggy Safari versions using IndexedDB.

[jamesmfriedman]

Got it. I’ll put in a PR with appropriate safari detection.

[Makatumba]

this check also causes Errors in our project, which relies on cordova-plugin-wkwebview-ionic-xhr. cordova-plugin-wkwebview-ionic-xhr uses a fetch polyfill

[nawazMoEngage]

is it fixed? if not, then any plans?

[AlexanderVagner]

We use ZoneJs in Angular project wich also polyfill fetch function, need better way to hasFetch function

[JustinSainton]

FWIW, this is causing intermittent issues for us as well - hasFetch is returning false when we're in Chrome 78.

[mareksuscak]

I found the root cause of the issue in the function that detects if isIndexDb is valid.

The issue here is that browsers can be emulating behaviors with polyfills. My specific issue is even more cryptic: My last pass extension in Chrome overrides window.fetch so it is no longer a native function.

function isIndexedDBValid() {
    try {
        // Initialize IndexedDB; fall back to vendor-prefixed versions
        // if needed.
        console.log('1');
        if (!idb) {
            return false;
        }
        // We mimic PouchDB here;
        //
        // We test for openDatabase because IE Mobile identifies itself
        // as Safari. Oh the lulz...
        var isSafari = typeof openDatabase !== 'undefined' && /(Safari|iPhone|iPad|iPod)/.test(navigator.userAgent) && !/Chrome/.test(navigator.userAgent) && !/BlackBerry/.test(navigator.platform);
        // THIS IS OVER PROTECTIVE. JUST LET A WINDOW.FETCH FUNCTION THROUGH
        var hasFetch = typeof fetch === 'function' && fetch.toString().indexOf('[native code') !== -1;

        // Safari <10.1 does not meet our requirements for IDB support (#5572)
        // since Safari 10.1 shipped with fetch, we can use that to detect it
        return (!isSafari || hasFetch) && typeof indexedDB !== 'undefined' &&
        // some outdated implementations of IDB that appear on Samsung
        // and HTC Android devices <4.4 are missing IDBKeyRange
        // See: https://github.com/mozilla/localForage/issues/128
        // See: https://github.com/mozilla/localForage/issues/272
        typeof IDBKeyRange !== 'undefined';
    } catch (e) {
        return false;
    }
}

You're right. This is extremely problematic because there are lots of libraries that polyfill fetch for various reasons, e.g. Fullstory and/or Sentry polyfill it for instrumentation.

[lincolnthree]

Any update on this? We are also running in to these issues :/

[tofumatt]

Not yet, but I can definitely understand this is a bit too protective. I'd happily accept a PR for this functionality, but I won't have time to cook one up myself until at least later this week.

If anyone can make a PR (preferably with some tests to cover polyfills), we should be able to ship this as a patch release.

[lincolnthree]

Am I correct in assuming that the following line:

var hasFetch = typeof fetch === 'function' && fetch.toString().indexOf('[native code') !== -1;

Should be replaced with:

var hasFetch = typeof fetch === 'function';

If so, happy to create a PR, though not really sure what adding test case would involve in this case. Suggestions?

[tofumatt]

I think that's it; probably best to have a test that polyfills window.fetch or just otherwise has it not pass the fetch.toString().indexOf('[native code') !== -1 check but still return true for isIndexedDBValid.

[lincolnthree]

Not sure I 100% followed you there. Here's the commit I've got so far.

https://github.com/lincolnthree/localForage/commit/6f8cfac2d688f5390bf2353133394bada87d69b4

Do you have any examples of how you'd want to polyfill in a test? Wouldn't that pollute the environment? (I've never polyfilled anything before so might be better if I left this to you.)

[tofumatt]

Hmm, our test code is so manual and old it's not a great place to test polyfills. 😓

I think your commit is a good start. If it passes the tests then we already aren't testing for that particular behaviour and I think it's fine to ship. If you wanted to start a PR with that change I think it'd be good, and I can look at adding a test—like you say—if it's straightforward.

Thanks!

[lincolnthree]

@tofumatt I know the feeling... At least you have tests ;)

PR submitted & linked.

[lincolnthree]

@tofumatt Hey! I hope all is well. Any chance we could get this reviewed/merged? I know life is busy but... this is still an issue. Just hoping :)

[lincolnthree]

Edit, I do see you've been making updates! Thank you!

[flipace]

@lincolnthree this issue is causing some hiccups for us as well 😅 any chance that PR is going to be merged soon?

[MHillier98]

Just commenting that I've just encountered this issue with LastPass as well.

[lincolnthree]

@flipace You'd have to ask @tofumatt ... but it seems like he's not looking at this.