Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Analytics: problem with "prevent initialization in unsupported environments" handling #3573

Closed
AoDev opened this issue Aug 6, 2020 · 7 comments · Fixed by #3836
Closed
Assignees

Comments

@AoDev
Copy link

AoDev commented Aug 6, 2020

Describe your environment

  • Operating System version: OSX Mojave
  • Browser version: Firefox in incognito (but probably any unsupported environment)
  • Firebase SDK version: 7.17.1
  • Firebase Product: analytics

Describe the problem

Errors are thrown when the environment is unsupported

Steps to reproduce:

  • Open a incognito window in Firefox
  • Try to initialize analytics (firebase.analytics())

Will throw:

Analytics: Environment doesn't support IndexedDB: A mutation operation was attempted on a database that did not allow mutations.. Wrap initialization of analytics in analytics.isSupported() to prevent initialization in unsupported environments (analytics/invalid-indexedDB-context)

Questions

  • Why does it need to throw an error in the first place? Can't this silently fail with a warning in the console.
    We get hundreds of thousands of error reports because of this.

  • Why do we have to run isSupported ourselves? can't the firebase lib check and initialize if it can? We could configure it to throw an error or just log a warning if some people do want to catch it but I think it would be better if it didn't break by default.

  • isSupported() is async, why? This makes it difficult to properly setup everything without delaying or queuing any call to analytics methods.

For example if it were not async we could just do something like

// myAnalytics.js
const analyticsMock = {
  logEvent: () => {},
  setCurrentScreen: () => {},
  setUserId: () => {},
}
export const analytics = analytics.isSupported() ? firebase.analytics() : analyticsMock
import analytics from 'myAnalytics'
...
myAnalytics.logEvent(...)
@google-oss-bot
Copy link
Contributor

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

@hsubox76
Copy link
Contributor

hsubox76 commented Aug 6, 2020

To answer your questions:

  1. If we did not throw an error, the browser itself would still throw an error (it's the "A mutation operation was attempted..." part up until "Wrap"). We are just proactively catching it ourselves so we can provide more info around how to prevent it.

  2. Why is it async: Because it needs to run IndexedDB.open() (a native browser method) to check if it can be run in the environment. Checking for its existence isn't good enough, in some environments it exists but throws a security error when actually run. Unfortunately IndexedDB.open() is itself async.

  3. Why can't Analytics do the check itself and not initialize if not found? It actually does do the check itself as well. Most of the environment checks it does are synchronous (like checking for cookies enabled) and skip the rest of the initialization process if they fail (by throwing). We could possibly make these warnings or info instead of errors.

    However the IndexedDB.open() check is different. Initialization of Analytics is synchronous by default and we didn't want to force it to be asynchronous in order to wait for the IndexedDB.open() check to complete. So we can't make that check block initialization. We thought at least it can throw an error message and tell the developer what to do next to prevent this (use isSupported()).

Unfortunately this makes the developer have to handle asynchronous initialization if they want to use isSupported() to ignore the errors.

We can look into a better way to address this. We can switch to using warnings for the sync checks, which already block initialization and won't lead to any further errors. Unfortunately it will be a little trickier to block the code that leads to the native browser error (which I'm sure you also don't want to see in your logs) after the async IndexedDB.open() check fails but there might be a way around it. Will try to look into these.

@AoDev
Copy link
Author

AoDev commented Aug 6, 2020

I see, thanks for all the details. I believe we should definitely do something at firebase level.

I am not sure how everyone codes their calls to analytics but usually the calls to log events will be "everywhere" in controllers and methods associated with user actions.

  • We need a mechanism for these methods to just "noop()" if analytics could not be initialized. (Maybe it's already the case?)

  • On the other hand, we need a mechanism to queue events, if we need to wait to know if analytics can be used. It would be better to have this at Firebase level than every developer having to make their own solution. For example, even before any user interaction, we have calls like "setUserId".

As an example of what it looks like on production, you can visit our app at https://battletraders.io with firefox in incognito. You will have an error report right away.

Screenshot 2020-08-06 at 21 55 48

@hsubox76
Copy link
Contributor

I will definitely look into doing a warning instead of an error when we can prevent initialization but just have one question: were you not getting errors from these same browsers before? This same error should have happened in all previous versions of analytics, except without being wrapped by Analytics text, so it would just say: "A mutation operation was attempted on a database that did now allow mutations." This operation has always been called when analytics initializes. The only change should have been that we now try this same operation before initialization and throw a more detailed error if it doesn't work.

@hsubox76 hsubox76 self-assigned this Aug 11, 2020
@AoDev
Copy link
Author

AoDev commented Aug 19, 2020

@hsubox76 Sorry for the late reply. I was out for a week.

I found an older error that confirms what you are saying.

InvalidStateError: A mutation operation was attempted on a database that did not allow mutations.

I also found this other error, cookie related this time, that is also throwing an error and rather be a warning.

Analytics: Cookies are not enabled in this browser environment. Analytics requires cookies to be enabled. (analytics/cookies-not-enabled).

Suggestion:

  • let analytics fail silently and/or with warning always, in case of unsupported env. (methods like logEvent just noop)
  • provide a way for developers to detect and react with isSupported() and/or a new method for backward compatibility, that could return the reason and not just a boolean.

The concept would be something like:

const {isSupported, reason} = await analytics.checkEnvSupport()
if (!isSupported) {
  if (reason === 'analytics/cookies-not-enabled') {
    alertUser('Please enable cookies')
  }
  // other reasons...
}

@salek-zylk
Copy link

salek-zylk commented Sep 9, 2020

I have a similar error. I am integrating an app of react with firebase (notifications and analytics) but analytics dont work. The browser is Chrome and the cookis are enabled
The console error is this:

provider.ts:108 Uncaught FirebaseError: Analytics: Cookies are not enabled in this browser environment. Analytics requires cookies to be enabled. (analytics/cookies-not-enabled).
at rt (https://www.gstatic.com/firebasejs/7.19.0/firebase-analytics.js:1:25586)
at _.instanceFactory (https://www.gstatic.com/firebasejs/7.19.0/firebase-analytics.js:1:27688)
at S.getOrInitializeService (https://www.gstatic.com/firebasejs/7.19.0/firebase-app.js:1:8327)
at S.getImmediate (https://www.gstatic.com/firebasejs/7.19.0/firebase-app.js:1:6591)
at q._getService (https://www.gstatic.com/firebasejs/7.19.0/firebase-app.js:1:13984)
at q.analytics (https://www.gstatic.com/firebasejs/7.19.0/firebase-app.js:1:17384)

@hsubox76
Copy link
Contributor

hsubox76 commented Sep 9, 2020

Addressing the cookie issue in the new issue you opened here: #3749

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants