firebase / firebase-js-sdk

Firebase Javascript SDK
https://firebase.google.com/docs/web/setup
Other
4.86k stars 894 forks source link

Incompatibility of 4.x with uglify #154

Closed richtera closed 6 years ago

richtera commented 7 years ago

[REQUIRED] Describe your environment

[REQUIRED] Describe the problem

Login using 3.6.x and upgrade the site to 4.1.x. Then try to enter the site. It will attempt to validate the saved token (in local storage) but fails. Somehow either through my code or internally to the firebase API it doesn't completely invalidate the token and thus keeps on attempting to use it when refreshing. Only clearing all browser local storage makes it work again.

Steps to reproduce:

See above.

Relevant Code:

I have not dug into it yet, but it's appearing as a problem on our upgraded site for end users.

http://jsbin.com/rinilu/edit?js,console

// TODO(you): code here to reproduce the problem
bojeil-google commented 7 years ago

I don't understand your explanation. Can you include console error messages or other visible errors generated. A refresh token is not invalidated by any client side version change.

richtera commented 7 years ago

Not sure if the error has much meaning. It shows up as:

Uncaught TypeError: a.qn is not a function
    at new t (vendor-4973e3b8.js:97)
    at vendor-4973e3b8.js:97
    at N (vendor-4973e3b8.js:94)
    at t.open (vendor-4973e3b8.js:97)
    at vendor-4973e3b8.js:98
    at r (vendor-4973e3b8.js:309)

Essentially some kind of token validation function is failing and throwing an error. I think the 3.6 firebase token items in local storage are different than the 4.1 ones and it's causing an exception when 4.1 tries to load and decode the old local storage item saved by 3.6.

richtera commented 7 years ago

The only other "visible" thing is that the site does not load and the end user just sees an empty page until they clear the cache and refresh.

bojeil-google commented 7 years ago

The error you provided could be anything as you are obfuscating it. 4.x introduced breaking changes, some APIs were removed or renamed from 3.x. This could be related to that.

richtera commented 7 years ago

I agree that this is the case, the problem is that it should not use the same local storage item name in that case. I can't really detect whether there was an old token when calling onTokenIdChanged and therefore don't know to delete such token. Is there a suggested way to make this safe from corrupted local store or old formatted local store? I am thinking it should really do this internally and ignore invalid tokens.

richtera commented 7 years ago

Trying to reproduce using an unobfuscated version...

bojeil-google commented 7 years ago

I just tested with simple logic:

// Initialize Firebase
var config = {
  apiKey: "...",
  authDomain: ..."
};
firebase.initializeApp(config);
firebase.auth().onAuthStateChanged(function(user) {
  if (user) {
    console.log(user);
  } else {
    console.log('no user logged in');
    firebase.auth().signInWithEmailAndPassword(email, password)
      .then(function(user) {
        console.log(user.uid);
      });
  }
});

I signed in a user with email/password in 3.6.0 I then replaced 3.6.0 with 4.3.0. The user was still logged in in 4.3.0 with no errors.

richtera commented 7 years ago

Hmmm, the problem only shows up when minifying the code. Digging a bit more.

richtera commented 7 years ago

Ok I cannot reproduce this at will. It happened on each of our production sites and I am still getting these errors from various users through Sentry but I am unable to setup the same problem locally easily. There might be another variable affecting this.

richtera commented 7 years ago

The stacktrace in Sentry is a bit more expressive but still not sure how to make sure it doesn't happen: image

richtera commented 7 years ago

Well the line it's complaining about it line 420 in BrowserPollConnection.ts

      //Create an iframe for us to add script tags to.
      this.myIFrame = FirebaseIFrameScriptHolder.createIFrame_();

Which doesn't shed much more light on it either though. It might be some kind of load order problem with FirebaseIFrameScriptHolder not being available during the first poll. Just confused.

richtera commented 7 years ago

I can reproduce the problem by transferring the firebase local store item key and value to another machine from one that fails.

richtera commented 7 years ago

Ok found out two things.

  1. 4.3 fixes the problem

  2. 4.1.x causes problems when being minified.

  3. Turning off minification of the vendor files (using uglify) also solves the problem.

So I think this can be closed, but if users are using gulp, bower and uglify they might run into this problem.

richtera commented 7 years ago

Not 100% reliable fix. Trying to turn of uglify for both 4.1 and 4.3 seems to be required. Problem is that gulp doesn't see any changes due to file content not changing. Therefore even rebuilding doesn't make it work yet. Once there is a new version of firebase in bower this should work but I am not quite sure how to force it yet.

richtera commented 7 years ago

Ok works now. So essentially the firebase.js file is incompatible with uglify's mangle option. The old 3.6 didn't have this problem but the new one does. I agree that we should not necessarily minify vendor files, but ideally this should not cause a problem. NOTE: This will only show up on a hosted site that needs to use an iFRAME for authentication. It will not cause any issues when running "localhost".

cmartin81 commented 7 years ago

I also have the same problem. I have an iconic 3 app, that works great when it is NOT minified. But when I build the application with the --prod flag it fails with the errormessage: auth/invalid-api-key. Then --prod flag minifies the code. I use firebase 4.3.1 library

bojeil-google commented 7 years ago

The Auth code has a check as follows when the Auth instance is internally instantiated:

if (this.app_().options && this.app_().options['apiKey']) {
  ..
} else {
  throw auth/invalid-api-key
}

App provides the options property. I speculate you are not minifying them together (the auth and app modules), or you are overloading one of the modules later or something of the like. It is impossible for me to guess without more details.

richtera commented 7 years ago

I was just minifying the whole firebase.js file to create this problem. Something about the iframe generator class causes minify to override the class with a local function name.

bojeil-google commented 7 years ago

There are 2 issues here and they seem to be different: @richtera and @cmartin81 My last answer is related to the latter. As for the former, this iframe generator class is part of database. Which seems to rule out Auth as the culprit. Auth has a separate mechanism for embedding its iframe. It is loaded dynamically too so it should not affect you.

richtera commented 7 years ago

I agree that I can't talk to all problems but my gut feeling is that I am seeing minified class names and minified method names on dynamically loaded code leads to problems when re-minifying code. The one instance I found was

t.ac

Changing to

a.ac

After the second minify and 'a' being a method instead of the iframe creator class. Therefore 'ac' was no longer bound to createIFrame.

jshcrowthe commented 7 years ago

@richtera are you enabling UglifyJS' mangleProps option? That is known to break code. However we also uglify with the mangle option, we are just conservative about which properties we mangle.

See:

https://github.com/firebase/firebase-js-sdk/blob/master/gulp/tasks/build.js#L166-L179