Address a performance regression from #4230 by deferring new URL computations.

src/utils/__tests__/avatar-test.js

@@ -71,12 +71,22 @@ describe('GravatarURL', () => {
     });
   });
 
-  test('uses hash from server, if provided', () => {
+  test('uses URL from server, if provided', () => {
     const email = 'user13313@chat.zulip.org';
-    const hash = md5('cbobbe@zulip.com');
-    const instance = GravatarURL.validateAndConstructInstance({ email, hash });
+    const urlFromServer =
+      'https://secure.gravatar.com/avatar/de6685f1d3eb74439c1dcda84f92543e?d=identicon&version=1';
+    const instance = GravatarURL.validateAndConstructInstance({
+      email,
+      urlFromServer,
+    });
     SIZES_TO_TEST.forEach(size => {
-      expect(instance.get(size).toString()).toContain(hash);
+      const clonedUrlOfSize = new URL(instance.get(size).toString());
+      const clonedUrlFromServer = new URL(urlFromServer);
+      clonedUrlFromServer.searchParams.delete('s');
+      clonedUrlOfSize.searchParams.delete('s');
+      // `urlFromServer` should equal the result for this size, modulo
+      // the actual size param.
+      expect(clonedUrlOfSize.toString()).toEqual(clonedUrlFromServer.toString());
     });
   });
 

src/utils/avatar.js

@@ -3,7 +3,6 @@
 /* eslint-disable no-use-before-define */
 import md5 from 'blueimp-md5';
 
-import { tryParseUrl } from './url';
 import * as logging from './logging';
 import { ensureUnreachable } from '../types';
 
@@ -53,14 +52,17 @@ export class AvatarURL {
       // which to generate the correct hash; see
       // https://github.com/zulip/zulip/issues/15287. Implemented at
       // `do_events_register` in zerver/lib/events.py on the server.)
-      if (tryParseUrl(rawAvatarUrl)?.origin === GravatarURL.ORIGIN) {
-        const hashMatch = /[0-9a-fA-F]{32}$/.exec(new URL(rawAvatarUrl).pathname);
-        if (hashMatch === null) {
-          const msg = 'Unexpected Gravatar URL shape from server.';
-          logging.error(msg, { value: rawAvatarUrl });
-          throw new Error(msg);
-        }
-        return GravatarURL.validateAndConstructInstance({ email, hash: hashMatch[0] });
+      if (
+        rawAvatarUrl.startsWith(
+          // Best not to use an expensive `new URL` call, when the
+          // following is equivalent (assuming `rawAvatarUrl` is also
+          // valid through its /pathname, ?query=params, and
+          // #fragment). The trailing slash ensures that we've
+          // examined the full origin in `rawAvatarUrl`.
+          `${GravatarURL.ORIGIN}/`,
+        )
+      ) {
+        return GravatarURL.validateAndConstructInstance({ email, urlFromServer: rawAvatarUrl });
       }
 
       // Otherwise, it's a realm-uploaded avatar, either absolute or
@@ -109,16 +111,46 @@ export class GravatarURL extends AvatarURL {
   /**
    * Construct from raw server data, or throw an error.
    *
-   * Pass the hash if the server provides it, to avoid computing it
-   * unnecessarily.
+   * Pass the Gravatar URL string from the server, if we've got it, to
+   * avoid doing an expensive `new URL` call.
    */
-  static validateAndConstructInstance(args: {| email: string, hash?: string |}): GravatarURL {
-    const { email, hash = md5(email.toLowerCase()) } = args;
-
-    const standardSizeUrl = new URL(`/avatar/${hash}`, GravatarURL.ORIGIN);
-    standardSizeUrl.searchParams.set('d', 'identicon');
+  // We should avoid doing unnecessary `new URL` calls here. They are
+  // very expensive, and their use in these pseudo-constructors (which
+  // process data at the edge, just as it's received from the server)
+  // used to slow down `api.registerForEvents` quite a lot.
+  //
+  // In the past, we've been more conservative about validating a URL
+  // string that comes to us from the server at this point: we'd parse
+  // it with the URL constructor, grab the Gravatar hash from the
+  // result, and construct another URL with that hash and exactly the
+  // things we wanted it to have (like `d=identicon`).
+  //
+  // With some loss of validation, we've removed those two `new URL`
+  // calls in the path where the server provides a Gravatar URL
+  // string. Still, we should be able to trust that the server gives
+  // us properly formatted URLs; if it didn't, it seems like the kind
+  // of bug that would be fixed quickly.
+  static validateAndConstructInstance(args: {|
+    email: string,
+    urlFromServer?: string,
+  |}): GravatarURL {
+    const { email, urlFromServer } = args;
 
-    return new GravatarURL(standardSizeUrl);
+    if (urlFromServer !== undefined) {
+      // This may not be *quite* the URL we would have generated
+      // ourselves. In the wild, I've seen one with a `version=1`, for
+      // example. But we trust the server to give us one that works,
+      // anyway -- and perhaps any extra things we get will be a good
+      // bonus.
+      return new GravatarURL(urlFromServer);
+    } else {
+      return new GravatarURL(
+        // Thankfully, this string concatenation is quite safe: we
+        // know enough about our inputs here to compose a properly
+        // formatted URL with them, without using `new URL`.
+        `${GravatarURL.ORIGIN}/avatar/${md5(email.toLowerCase())}?d=identicon`,
+      );
+    }
   }
 
   static ORIGIN = 'https://secure.gravatar.com';
@@ -195,32 +227,36 @@ export class FallbackAvatarURL extends AvatarURL {
   }
 
   /**
-   * Construct from raw server data, or throw an error.
+   * Construct from raw server data (the user ID), or throw an error.
+   *
+   * The `realm` must be already validated, e.g., by coming from the
+   * Redux state.
    */
+  // We should avoid doing unnecessary `new URL` calls here. They are
+  // very expensive, and their use in these pseudo-constructors (which
+  // process data at the edge, just as it's received from the server)
+  // used to slow down `api.registerForEvents` quite a lot.
   static validateAndConstructInstance(args: {| realm: URL, userId: number |}): FallbackAvatarURL {
     const { realm, userId } = args;
-    return new FallbackAvatarURL(new URL(`/avatar/${userId.toString()}`, realm));
+    // Thankfully, this string concatenation is quite safe: we know
+    // enough about our inputs here to compose a properly formatted
+    // URL with them, without using `new URL`. (In particular,
+    // `realm.origin` doesn't have a trailing slash.)
+    return new FallbackAvatarURL(`${realm.origin}/avatar/${userId.toString()}`);
   }
 
   /**
    * Standard URL from which to generate others. PRIVATE.
    *
-   * May be a string if the instance was constructed at rehydrate
-   * time, when URL validation is unnecessary.
+   * May start out as a string, and will be converted to a URL object
+   * in the first `.get()` call.
    */
   _standardUrl: string | URL;
 
   /**
    * PRIVATE: Make an instance from already-validated data.
    *
    * Not part of the public interface; use the static methods instead.
-   *
-   * It's private because we need a path to constructing an instance
-   * without constructing URL objects, which takes more time than is
-   * acceptable when we can avoid it, e.g., during rehydration.
-   * Constructing URL objects is a necessary part of validating data
-   * from the server, but we only need to validate the data once, when
-   * it's first received.
    */
   constructor(standardUrl: string | URL) {
     super();
@@ -238,7 +274,7 @@ export class FallbackAvatarURL extends AvatarURL {
    */
   get(sizePhysicalPx: number): URL {
     // `this._standardUrl` may have begun its life as a string, to
-    // avoid computing a URL object during rehydration
+    // avoid expensively calling the URL constructor
     if (typeof this._standardUrl === 'string') {
       this._standardUrl = new URL(this._standardUrl);
     }