cyrilletuzi / angular-async-local-storage

Efficient client-side storage for Angular: simple API + performance + Observables + validation
MIT License
675 stars 68 forks source link

Object store missing in Firefox on very first use #95

Closed FinnStutzenstein closed 2 years ago

FinnStutzenstein commented 5 years ago

Hi,

we ran into problems with Firefox (not private mode, 60.3.0esr and 66.0 tested):

Error: Uncaught (in promise): NotFoundError: The operation failed because the requested database object could not be found. For example, an object store did not exist but was being opened.

Minimal steps to reproduce: Install angular CLI: npm install @angular/cli, create new project: ng new firefox-IDB-test (with routing, scss as style choosen), install localStorage: npm install @ngx-pwa/local-storage@6. This is the content of the app.component.ts:

import { Component } from '@angular/core';

import { LocalStorage } from '@ngx-pwa/local-storage';

@Component({
  selector: 'app-root',
  templateUrl: './app.component.html',
  styleUrls: ['./app.component.scss']
})
export class AppComponent {
    public title = 'firefox-IDB-test';

    public constructor(protected localStorage: LocalStorage) {
        this.doIt();
    }

    public async doIt(): Promise<void> {
        let val = await this.localStorage.getItem<string>('myKey').toPromise();
        console.log('Read: ', val);
        if (!val) {
            val = 'Test string';
        }

        try {
            await this.localStorage.setItem('myKey', val).toPromise();
            console.log('set item successfully');
        } catch (e) {
            console.log('Error setting item: ', e);
        }
    }
}

This does nothing more than reading a value and write it back. Expected behaviour: null on first read, 'Test string' on successive reads. Note: This happens not every time, in my case about every 4th try. I clean everything in the storage-tab and reopen the browser.

I traced this a bit down: Sometimes the upgradeneeded is not fired from the IDBOpenDBRequest. This results in a missing object store. In e.g. Chrome or Edge this event is always fired. The docs says that on every version change and DB creation (which is the case here), this event should be fired.

I'm not sure if this is a Firefox bug. If so maybe you can investigate a bit deeper. A possible solution would be to open a versionchange transaction and create the object store again if it does not exist...

Thanks!

FinnStutzenstein commented 5 years ago

Some more information: To work around this, I made a custom DB-class, that inherits the IndexedDBDatabase and added this code inside the open-request's success-handler:

const db = (event.target as IDBRequest).result as IDBDatabase;

// CUSTOM: If the indexedDB initialization fails, because 'upgradeneeded' didn't fired
// the fallback will be used.
if (!db.objectStoreNames.contains(this.objectStoreName)) {
    this.setFallback(prefix);
} else {
    // Storing the database connection for further access
    this.database.next(db);
    this.storageLock.OK();
}

Another thing: I noticed we are using the LocalStorage (e.g. getItem, setItem, ...) before the success event was fired. This happens, if in some constructors of services, the LocalStorage is used. Can you provide an event, that signals the completion of the setup process, so my application can wait until the LocalStorage is initialized? In this case the this.storageLock.OK(); is the quick solution for this. I also added this to the setFallback method:

public setFallback(prefix: string): void {
    super.setFallback(prefix);
    this.storageLock.OK();
}

Maybe the reported issue (not firing upgradeneeded) was a consequence of this use-case. I'll check this the next days.

cyrilletuzi commented 5 years ago

Thanks for your interest for this lib and your detailed feedback.

Upcoming v8 will be a nearly full rewrite of the lib, and pretends to fix such edgy cases. So could you test with v8 beta and tell me if your problems still happen? You'll find instructions in #76.

FinnStutzenstein commented 5 years ago

Hi, with v8 the same error occurs. I looked at the new code and the main part is the same. I tried to plug the custom changes I made yesterday into it. Since the visibility was changed to private everywhere, I had to copy the whole IDBDatabase code. Also I couldn't finished testing, because not every exception and token is provided in the public API. Currently there is no way for an application that uses v8, to modify small pieces of it. This would be nice to have if someone wants to plug in application specific code. Also a "database was setup successfully"-event would be nice to have. I'll test with v7, if the "wait for the DB befor making a first transaction" helps..

cyrilletuzi commented 5 years ago

Thanks for testing. As it's still there in v8, I'll try to reproduce your issue. I'll keep you in touch.

Also I couldn't finished testing, because not every exception and token is provided in the public API.

Can you elaborate on this? One goal of the v8 rewrite is a better error management. I reviewed the full lib to manage errors everywhere some error can happen.

Currently there is no way for an application that uses v8, to modify small pieces of it.

Such behavior was possible before v8 but never documented, because the very purpose of the lib is to be a wrapper around indexedDB, so the user doesn't have to deal with the native API complexity. Also, introducing specific things with the complex indexedDB API could lead to unexpected behavior.

That's why it's now restricted in v8. I could revert this, but if there are good reasons. For what we are currently speaking, it would be a bad reason: it's an issue, so the good way is not for the user to add some specific code, but the issue to be fixed in the lib.

FinnStutzenstein commented 5 years ago

Also I couldn't finished testing, because not every exception and token is provided in the public API.

Can you elaborate on this? One goal of the v8 rewrite is a better error management. I reviewed the full lib to manage errors everywhere some error can happen.

Not all tokens and exceptions are exported in the public_api.ts, for instance the LS_PREFIX. These are needed if one want to implement custom things (see below).

That's why it's now restricted in v8. I could revert this, but if there are good reasons. For what we are currently speaking, it would be a bad reason: it's an issue, so the good way is not for the user to add some specific code, but the issue to be fixed in the lib.

I agree with you, but maybe there are other purposes where a modifiable library would be nice. E.g. in the old version you had to modify the IndexedDBDatabase to add other fallback strategies (In v8 it's not more necessary :+1: ). I, as a user of some libraries, like it to know, that I can modify things. Even if it's only for fuzzing around, like in this case and even I know, that it is not recommended.

Sometimes you have to override things, because the maintainer won't change things just for you and your special usecase.

But I do not know how many users of this library are affected by the fact, that it can't be modified with v8. I guess not much to none.. We in our project (https://github.com/OpenSlides/OpenSlides) didn't needed to change anything (until this Firefox issue).

TL;DR I do not have any good reasons, but only for convenience ;)

Thanks for investigating deeper!

cyrilletuzi commented 5 years ago

Unfortunately I can't reproduce your issue. I added an e2e test in v8 with usage in a service, and it works. I've also done that from a long time in my own apps.

So I will need a full reproduction of the issue (on Stackblitz for example).

FinnStutzenstein commented 5 years ago

Hi, with stackblitz, I cant reproduce this issue. Maybe it has something to do with loading times, because the actual stackblitz page is loaded much more early than the actual app?

Nevertheless I made a repository: https://github.com/FinnStutzenstein/firefox-IDB-test Just do an npm install and npm run start. In this repo is a firefox.mp4 where I shortly recorded my way to reproduce it about every second time.

Clear everything (Only indexed DB should be sufficient, but who knows..) and clear the ngStorage first, then the localStorage. Open a new tab, close the old one. restart firefox. open localhost:4200 and the dev console.

Maybe this helps..

cyrilletuzi commented 5 years ago

Thanks for the details. I've done some investigation this week but still couldn't find the isssue. For info:

I'll spend more time on this later.

cyrilletuzi commented 5 years ago

@FinnStutzenstein Hi, is there anything new about the issue on your side?

I still can't reproduce this issue in a normal context. In your video demo, you're manually modifying the storage in dev tools, which will not happen to a normal user, and if you delete the store (localStorage) but not the database (ngStorage), it's normal that the lib fails.

Still, I've been thinking over and over about how to fix this issue nevertheless, and I can't find a way to do it in a proper way. Trying to recreate the store would mean changing the default database name and/or version at any moment, which would mess up things, particulary in v8 where these informations can be retrieved from the lib to allow interoperability with other libs or native indexedDB.

I've also looked at other libs: the kv-storage polyfill, which is an upcoming official standard and done by experts, just throws if the store is missing. So it seems there is no fix possible for this issue...

Let me know if you have any idea.

FinnStutzenstein commented 5 years ago

I will see if I can still reproduce the issue this week and will notify you.

FinnStutzenstein commented 5 years ago

Hi,

I still can't reproduce this issue in a normal context. In your video demo, you're manually modifying the storage in dev tools, which will not happen to a normal user,

Unfortunately, this was reported by clients using our software. This may happen on the first ever load of the application, so this "clearing" should reproduce this scenario.

if you delete the store (localStorage) but not the database (ngStorage), it's normal that the lib fails.

Maybe this can be checked during initialization?

I added some logging abilities in our software to query, how many users did have this issue. I'll track these numbers a bit.

I still can reproduce this in 60.3esr, but not in 67.0esr, which is the newest Firefox. My guess would be an issue in Firefox, as strange as it sounds, even they may make issues. So I think you shouldn't investigate any more. Thanks for your support here!

cyrilletuzi commented 5 years ago

Thanks for your feedback.

Firefox do have issues with indexedDB (like #26).

I'll leave this issue opened in case someone has more information.

sebaherrera commented 5 years ago

Hi. I found that error message with some frequency and noticed that, in most cases, visitor came from Instagram app (in fact, Chrome or Safari web view) and occurs generally in the first 5 seconds of runtime (maybe when calling localStorage.lenght).

Here is one of the stack traces I've managed to retrieve via Rollbar:

File webpack:///./node_modules/@ngx-pwa/local-storage/fesm2015/ngx-pwa-local-storage.js.pre-build-optimizer.js line 426 col 37 in Object.next if (!request.result.objectStoreNames.contains(this.storeName)) { File webpack:///./node_modules/rxjs/_esm2015/internal/Subscriber.js.pre-build-optimizer.js line 183 col 16 in f.__tryOrUnsub fn.call(this._context, value); File webpack:///./node_modules/rxjs/_esm2015/internal/Subscriber.js.pre-build-optimizer.js line 122 col 22 in f.next this.__tryOrUnsub(this._next, value); File webpack:///./node_modules/rxjs/_esm2015/internal/Subscriber.js.pre-build-optimizer.js line 72 col 26 in g._next this.destination.next(value); File webpack:///./node_modules/rxjs/_esm2015/internal/Subscriber.js.pre-build-optimizer.js line 49 col 18 in g.next this._next(value); File webpack:///./node_modules/rxjs/_esm2015/internal/operators/throwIfEmpty.js.pre-build-optimizer.js line 24 col 26 in La._next this.destination.next(value); File webpack:///./node_modules/rxjs/_esm2015/internal/Subscriber.js.pre-build-optimizer.js line 49 col 18 in La.next this._next(value); File webpack:///./node_modules/rxjs/_esm2015/internal/operators/take.js.pre-build-optimizer.js line 35 col 30 in Ka._next this.destination.next(value); File webpack:///./node_modules/rxjs/_esm2015/internal/Subscriber.js.pre-build-optimizer.js line 49 col 18 in Ka.next this._next(value); File webpack:///./node_modules/rxjs/_esm2015/internal/observable/fromEvent.js.pre-build-optimizer.js line 17 col 28 in IDBOpenDBRequest. subscriber.next(Array.prototype.slice.call(arguments));

My app is based on version 8.0.2 in Angular 8.1.2 with default v8 setup but legacy code (localStorage, not StorageMap yet) on function calls.

Hope this helps on debugging.

cyrilletuzi commented 5 years ago

Hi @sebaherrera, thanks for your feedback.

Do you confirm it happens only in Firefox?

Unfortunately, the stack trace doesn't help. Do you have a reproduction of the issue?

but legacy code (localStorage, not StorageMap yet) on function calls. As a side note, LocalStorage service is still OK, all APIs use the same code internally, it's just different public API surfaces.

sebaherrera commented 5 years ago

Sorry, I missed https://github.com/cyrilletuzi/angular-async-local-storage/issues/95#issuecomment-498133034. It may be related to https://bugzilla.mozilla.org/show_bug.cgi?id=1423917 because it fits perfectly on bug timeframe.

I thought that my information could be of help since it's the same exception, but mine seems related specifically to Instagram app, and maybe when it fetches metadata. If I manage to reproduce the issue, I'll be in touch.

jamajamik commented 4 years ago

Hello, I am experiencing the same issue. Chrome seems OK. Only appears randomly on Firefox (tested version 56 and 71). When testing locally is not so often as when opening from remote server. I haven't time to dig in more details. My impression is like the storageMap.set execution is not corretly waiting for IndexDB to be initialized - mainly createObjectStore finished. Seems like it depends how early is the storagemap.set() called. In my scenario it is one of first action. Also note muliple set() method is fired. Maybe some async init code have not yet finished....

cyrilletuzi commented 4 years ago

@jamajamik Thanks for your report.

Unfortunately, like you say, the issue happens randomly. I've seen it myself, but I was never able to reproduce it, despite having tried to debug this issue dozen of times.

I've also checked dozen of times the lib code: things happen in the right order from the code side. If not, we would have seen some issues at some times in other browsers too.

So having search a lot about this, I've come to the conclusion it's a Firefox issue (it's not the only one, like #26, where the issue in Firefox bug tracker is opened from 7 years now).

Having also read a lot about this subject, I've seen that browsers recently did optimizations on storage APIs, and that could be the issue (it's even stated clearly on MDN).

So except there is new elements (reproduction, idea of workarounds...), I won't be able to do anything about this issue.

My only hope is the upcoming kv-storage standard. That would force browsers to see the flaws of their own implementations of indexedDb, and to fix them.

jamajamik commented 4 years ago

@cyrilletuzi Thanks for prompt reply. As the issue appears pretty frequently in my code I will elaborate litlle bit and try to make some isolated reproducable code. But it takes few weeks before I have some spare time for that hunt...

Would be possible to make following workaround: Catch exception, force createObjectStore and retry? I am not familiar with indexDB API yet. So I am not sure about exact code and where too hook it at best. Something like

catchError( 
 return forceObjectStoreCreate().pipe(doRetry)
 )  

Did @FinnStutzenstein workaround resolved the issue with older version? Is it possible to use it with current library?

cyrilletuzi commented 4 years ago

Catch exception, force createObjectStore and retry?

Not possible. Trying to recreate the object store would need to retrigger the upgradeneeded event, ie. it would require to bump the database version number, which would override the version that lib user can choose. The logic of the upgradeneeded event is a real pain.

sebaherrera commented 4 years ago

Hi. What about falling back into localStorage or memory? It would be consistent with the general behaviour of the library.

FinnStutzenstein commented 4 years ago

@jamajamik The provided modification did not solve the problem. It was just a way to detect and catch the error and do a fallback. This fallback was implemented back then, but for the current version, I'm not sure, if this still exists. The snippet does exactly what @sebaherrera said above, but I'm not sure, if the library still has the fallback mechanism etc.

cyrilletuzi commented 4 years ago

What about falling back into localStorage or memory?

If the issue was always happening in a specific scenario we could do that, but as the issue happens randomly, it would mean that a user arriving on the app with the issue would have his/her data stored in localStorage, and then the next time he/she opens the app the lib would init with indexedDb and thus data from the previous visit would be missing.

cyrilletuzi commented 2 years ago

This issue is now 2 years old, and Firefox has fixed a lot of indexedDb issues since. Is someone still having this problem? Otherwise I'll close the issue.

cyrilletuzi commented 2 years ago

Closing the issue then, feel free to open a new one if needed.