fix(sign_in): Fix how the listener is bound for account->chang:accessToken (#5103) r=@philbooth

We were binding using `account.on` in `beforeRender`. This didn't cause any
visible problems, but caused two problems behind the scenes:

1. The view to be retained in memory after being torn down because the
   account kept a reference to the View.
2. If the accessToken was invalidated after the view was destroyed, the
   *view would re-render anyways*, just not visibly.

Using view.listenTo, the listener is removed from the account whenever
the view is torn down. In addition, this is only done in afterVisible
so that only one listener is ever bound. If done in `beforeRender`,
a new listener was attached on every render.

Not attached to an issue.

app/scripts/views/sign_in.js

@@ -9,7 +9,6 @@ define(function (require, exports, module) {
   const allowOnlyOneSubmit = require('views/decorators/allow_only_one_submit');
   const AuthErrors = require('lib/auth-errors');
   const AvatarMixin = require('views/mixins/avatar-mixin');
-  const BaseView = require('views/base');
   const Cocktail = require('cocktail');
   const ExperimentMixin = require('views/mixins/experiment-mixin');
   const FlowBeginMixin = require('views/mixins/flow-begin-mixin');
@@ -18,6 +17,7 @@ define(function (require, exports, module) {
   const MigrationMixin = require('views/mixins/migration-mixin');
   const PasswordMixin = require('views/mixins/password-mixin');
   const PasswordResetMixin = require('views/mixins/password-reset-mixin');
+  const { preventDefaultThen, t } = require('views/base');
   const ResumeTokenMixin = require('views/mixins/resume-token-mixin');
   const ServiceMixin = require('views/mixins/service-mixin');
   const Session = require('lib/session');
@@ -26,15 +26,13 @@ define(function (require, exports, module) {
   const SignInMixin = require('views/mixins/signin-mixin');
   const SignInTemplate = require('stache!templates/sign_in');
 
-  var t = BaseView.t;
+  const proto = FormView.prototype;
 
-  var View = FormView.extend({
+  const View = FormView.extend({
     template: SignInTemplate,
     className: 'sign-in',
 
-    initialize (options) {
+    initialize (options = {}) {
-      options = options || {};
-
       this._formPrefill = options.formPrefill;
 
       // The number of stored accounts is logged to see if we can simplify
@@ -49,18 +47,25 @@ define(function (require, exports, module) {
 
     beforeRender () {
       this._account = this._suggestedAccount();
+    },
 
-      this._account.on('change:accessToken', () => {
+    afterVisible () {
+      proto.afterVisible.call(this);
+      // this.displayAccountProfileImage could cause the existing
+      // accessToken to be invalidated, in which case the view
+      // should be re-rendered with the default avatar.
+      const account = this.getAccount();
+      this.listenTo(account, 'change:accessToken', () => {
         // if no access token and password is not visible we need to show the password field.
-        if (! this._account.has('accessToken') && this.$('.password').is(':hidden')) {
+        if (! account.has('accessToken') && this.$('.password').is(':hidden')) {
           // accessToken could be changed async by an external request after render
           // If the ProfileClient fails to get an OAuth token with the current token then reset the view
           this.chooserAskForPassword = true;
-          return this.render().then(function () {
+          return this.render().then(() => this.setDefaultPlaceholderAvatar());
-            this.setDefaultPlaceholderAvatar();
-          });
         }
       });
+
+      return this.displayAccountProfileImage(account, { spinner: true });
     },
 
     getAccount () {
@@ -111,11 +116,6 @@ define(function (require, exports, module) {
       'click .use-logged-in': 'useLoggedInAccount'
     },
 
-    afterVisible () {
-      FormView.prototype.afterVisible.call(this);
-      return this.displayAccountProfileImage(this.getAccount(), { spinner: true });
-    },
-
     beforeDestroy () {
       this._formPrefill.set('email', this.getElementValue('.email'));
       this._formPrefill.set('password', this.getElementValue('.password'));
@@ -195,7 +195,7 @@ define(function (require, exports, module) {
     /**
      * Render to a basic sign in view, used with "Use a different account" button
      */
-    useDifferentAccount: BaseView.preventDefaultThen(function () {
+    useDifferentAccount: preventDefaultThen(function () {
       // TODO when the UI allows removal of individual accounts,
       // only clear the current account.
       this.user.removeAllAccounts();

app/tests/spec/views/sign_in.js

@@ -163,9 +163,11 @@ define(function (require, exports, module) {
           });
 
           sinon.stub(view, '_suggestedAccount', () => account);
+          sinon.stub(view, 'displayAccountProfileImage', () => p());
           sinon.spy(view, 'render');
 
           return view.render()
+            .then(() => view.afterVisible())
             .then(() => {
               account.set({
                 accessToken: null,
@@ -184,6 +186,38 @@ define(function (require, exports, module) {
       });
     });
 
+    describe('destroy', () => {
+      beforeEach(() => {
+        initView();
+
+        const account = user.initAccount({
+          accessToken: 'access token',
+          email: 'a@a.com',
+          sessionToken: 'session token',
+          sessionTokenContext: Constants.SYNC_SERVICE
+        });
+
+        sinon.stub(view, '_suggestedAccount', () => account);
+        sinon.stub(view, 'displayAccountProfileImage', () => p());
+        sinon.spy(view, 'render');
+
+        return view.render()
+          .then(() => view.afterVisible())
+          .then(() => view.destroy())
+          .then(() => {
+            account.set({
+              accessToken: null,
+              sessionToken: null,
+              sessionTokenContext: null
+            });
+          });
+      });
+
+      it('does not re-render once destroyed if the accessToken is invalidated', () => {
+        assert.equal(view.render.callCount, 1);
+      });
+    });
+
     describe('migration', () => {
       it('does not display migration message if no migration', () => {
         initView();