Prevent errors/crashing when multiple installs of DevTools are present

[jstejada]

Summary

This commit is a proposal for handling duplicate installation of DevTools, in particular scoped to duplicates such as a dev build or the internal versions of DevTools installed alongside the Chrome Web Store extension.

Specifically, this commit makes it so when another instance of the DevTools extension is installed alongside the extension installed from the Chrome Web Store, we don't produce a stream of errors or crash Chrome, which is what would usually happen in this case.

Detecting Duplicate Installations

• First, we check what type of installation the extension is: from the Chrome Web Store, the internal build of the extension, or a local development build.
• If the extension is from the Chrome Web Store:
  • During initialization, we first check if the internal or local builds of the extension have already been installed and are enabled. To do this, we send a cross-extension message to the internal and local builds of the extension using their extension IDs.
    • We can do this because the extension ID for the internal build (and for the Chrome Web Store) is a stable ID.
    • For the local build, at build time we hardcode a key in the manifest.json which allows us to have a stable ID even for local builds.
  • If we detect that the internal or local extensions are already installed, then we skip initializing the current extension altogether so as to not conflict with the other versions. This means we don't initialize the frontend or the backend at all.
• If the extension is the Internal Build:
  • During initialization, we first check if the local builds of the extension has already been installed and is enabled. To do this, we send a cross-extension message to the local build of the extension using its extension ID.
    • We can do this for the local build because at build time we hardcode a key in the manifest.json which allows us to have a stable ID even for local builds.
  • If we detect that the local extension is already installed, then we skip initializing the current extension altogether so as to not conflict with the that version. This means we don't initialize the frontend or the backend at all.
• If the extension is a Local Dev Build:
  • Since other extensions check for the existence of this extension and disable themselves if they detect it, we don't need any special handling during initialization and assume that there are no duplicate extensions. This means that we will generally prefer keeping this extension enabled.

This behavior means that the order of priority for keeping an extension enabled is the following:

1. Local build
2. Internal build
3. Public build

Preventing duplicate backend initialization

Note that the backend is injected and initialized by the content script listening to a message posted to the inspected window (via postMessage). Since the content script will be injected twice, once each by each instance of the extension, even if we initialize the extension once, both content scripts would still receive the single message posted from the single frontend, and it would then still inject and initialize the backend twice.

In order to prevent this, we also add the extension ID to the message for injecting the backend. That way each content script can check if the message comes from its own extension, and if not it can ignore the message and avoid double injecting the backend.

Other approaches

• I considered using the chrome.management API generally to detect other installations, but that requires adding additional permissions to our production extension, which didn't seem ideal.
• I also considered a few options of writing a special flag to the inspected window and checking for it before initializing the extension. However, it's hard to avoid race conditions in that case, and it seemed more reliable to check specifically for the WebStore extension, which is realistically where we would encounter the overlap.

Rollout

• This commit needs to be published and rolled out to the Chrome Web Store first.
• After this commit is published to the Chrome Web Store, any duplicate instances of the extension that are built and installed after this commit will no longer conflict with the Chrome Web Store version.

Next Steps

• In a subsequent PR, I will extend this code to show a warning when duplicate extensions have been detected.

Part of #22486

How did you test this change?

Basic Testing

• yarn flow
• yarn test
• yarn test-build-devtools

Double installation testing

Testing double-installed extensions for this commit is tricky because we are relying on the extension ID of the internal and Chrome Web Store extensions, but we obviously can't actually test the Web Store version (since we can't modify the already published version).

In order to simulate duplicate extensions installed, I did the following process:

• Built separate extensions where I hardcoded a constant for whether the extension is internal or public (e.g. EXTENSION_INSTALLATION_TYPE = 'internal'). Then I installed these built extensions corresponding to the "internal" and "Web Store" builds.
• Build and run the regular development extension (with yarn build:chrome:dev && yarn test:chrome), using the extension IDs of the previously built extensions as the "internal" and "public" extension IDs.

With this set up in place, I tested the following on pages both with and without React:

• When only the local extension enabled, DevTools works normally.

• When only the "internal" extension enabled, DevTools works normally.

• When only the "public" extension enabled, DevTools works normally.

• When "internal" and "public" extensions are installed, "public" extension is disabled and "internal" extension works normally.

• When the local extension runs alongside the other extensions, other extensions disable themselves and local build works normally.

• When we can't recognize what type of build the extension corresponds to, we show an error:

• When all 3 extensions are installed and enabled in all different combinations, DevTools no longer produces errors or crashes Chrome, and works normally:

Before
If it doesn't crash, would produce these errors:

After
No errors:

[jstejada]

fec96f8: rebase + resolve conflicts

[bvaughn]

If we detect that we are not the Web Store extension /and/ the Web Store extension is already installed, then we skip initializing the current extension altogether so as to not conflict with the Web Store version. This means we don't initialize the frontend or the backend at all.

I think this makes sense but is maybe the opposite of what we'd want to do in most cases.

For example, if Chef-installed extension is present, it would be better to use it (since internal built extensions are almost always newer and/or have extra features).

If a locally-built extension is present, we would want to use it for sure because we're probably testing a change. Using the store extension version only may cause us to think e.g. that a breaking change is fine.

[jstejada]

i'll make the following changes here:

• Flip the logic so the Chrome Web Store behavior disables itself and the others are allowed to run
• Verify the cases when there are 2 non-Web Store copies present, or when all 3 are present, and make sure that it still doesn't crash

[jstejada]

@bvaughn I made several updates to this diff to better handle the combinations of the 3 types of builds we'd most commonly encounter: Web Store, internal, and local dev build.

See the updated summary and test plan for an overview of the updated behavior

[bvaughn]

NOTE: Ideally in this case we would disable any other extensions except the development one. However, since we don't have a stable extension ID for dev builds, doing so would require for other installations to wait for a message from this extension, which would unnecessarily delay initialization of those extension

I don't think I understand this comment. Why do we need a stable ID for the locally built extension?

Wouldn't get and setEnabled be sufficient for us to detect and disable the internal and/or public builds if they're installed along side of a local build?

Basically the local build could check for and disable either the internal FB build or the public build if either were installed.

Then the internal build could detect and disable the public build if it was installed. (I think we should be okay to add the "management" permission to the internal build as well.)

Then the public build could stay as-is (without any new permissions or checks).

[jstejada]

I don't think I understand this comment. Why do we need a stable ID for the locally built extension?

Wouldn't get and setEnabled be sufficient for us to detect and disable the internal and/or public builds if they're installed along side of a local build?

Basically the local build could check for and disable either the internal FB build or the public build if either were installed.

Then the internal build could detect and disable the public build if it was installed. (We should be okay to add the "management" permission to the internal build as well.)

Then the public build could stay as-is (without any new permissions or checks).

Yeah, I think the main points here are:

• using getAll instead of get helps us detect and at least show some sort of warning for any straggler extensions that aren't the public or internal one
• setEnabled needs to be called from within a user interaction, e.g. an event like a click, so I can't directly disable them from the local dev build. without being able to call setEnabled, the only other option is to those other extensions to wait and see if the local build reaches out, right? let me know if that makes sense, maybe i'm missing something here
• I also think it's preferable to completely prevent initialization rather than disable at a later point with setEnabled, since in that case we might still allow for errors / crashes to occur

Side note: i like your naming of using public instead of "chrome web store" which is a mouthful, i'll update that in the code.

[bvaughn]

• setEnabled needs to be called from within a user interaction, e.g. an event like a click, so I can't directly disable them from the local dev build. without being able to call setEnabled, the only other option is to those other extensions to wait and see if the local build reaches out, right? let me know if that makes sense, maybe i'm missing something here

Ah, that's fair. And if we waited for a user interaction, the browser might already have locked up.

• I also think it's preferable to completely prevent initialization rather than disable at a later point with setEnabled, since in that case we might still allow for errors / crashes to occur

Agreed.

[jstejada]

[bvaughn]

Nice!