clients icon indicating copy to clipboard operation
clients copied to clipboard

[PM-5882] Adjust usage of `chrome.extension.getBackgroundPage` to ensure no conflicts exist due to the service worker

Open cagonzalezcs opened this issue 1 year ago • 2 comments

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

The objective of this PR is to ensure that usage of chrome.extension.getBackgroundPage does not alter expected logic and behavior within manifest v3. To that end, this was partially an investigative task, which yielded the following results:

  • chrome.extension.getBackgroundPage is leveraged explicitly BrowserApi.getBackgroundPage method, which in turn is called in a couple of locations. These include BrowserApi.isBackgroundPage, BrowserPopupUtils.backgroundInitializationRequired, and within services.module.ts.
  • BrowserApi.isBackgroundPage is called by BrowserApi.addListener, BrowserApi.removeListener, and the SessionSyncer class.
  • Usage of BrowserApi.isBackgroundPage in the BrowserApi.addListener and BrowserApi.removeListener methods should not be affected by the mv3 service worker, as the call is only made for Safari extensions. That said, I've removed usage of the window object and referenced that global value as self to fix any potential issues that might arise from calling the window.
  • Usage of BrowserApi.isBackgroundPage in the SessionSyncer class is handled within SessionSyncer.update and SessionSyncer.updateSession. SessionSyncer.update uses the BrowserApi.isBackgroundPage call to save a value to the passed MemoryStorageService if the extension is within manifest v2 and the call happens within the backgorund page. SessionSyncer.updateSession uses the BrowserApi.isBackgroundPage method to handle saving to the passed MemoryStorageService if the call happens in an manifest v3 extension or within the background page script. In both these instances, it seems that the implementation was put in place with the intention of having different behavior between manifest v2 and 3. Regardless of that implementation choice, it doesn't seem like calling BrowserApi.isBackgroundPage will cause problems in mv3.
  • Usage of BrowserPopupUtils.backgroundInitializationRequired seems to happen within the context of the vault popup only, and as a result it will not cause issues directly with mv3. More importantly however, the usage of this will be phased out entirely as the only location where this call is used is services.module.ts. It is a know requirement that we refactor usage of chrome.extension.getBackgroundPage and BrowserPopupUtils.backgroundInitializationRequired out of the implementation completely.

Code changes

  • apps/browser/src/platform/browser/browser-api.ts: Updating references to the window object to refer to the global self object instead.

cagonzalezcs avatar Feb 22 '24 21:02 cagonzalezcs

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 24.83%. Comparing base (929b5eb) to head (5f5cec4). Report is 1 commits behind head on main.

:exclamation: Current head 5f5cec4 differs from pull request most recent head 6067b06. Consider uploading reports for the commit 6067b06 to get more accurate results

Files Patch % Lines
apps/browser/src/platform/browser/browser-api.ts 0.00% 0 Missing and 2 partials :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8054   +/-   ##
=======================================
  Coverage   24.83%   24.83%           
=======================================
  Files        2233     2233           
  Lines       65511    65511           
  Branches    12363    12363           
=======================================
  Hits        16271    16271           
  Misses      47914    47914           
  Partials     1326     1326           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 22 '24 21:02 codecov[bot]

Logo Checkmarx One – Scan Summary & Details1cb194d5-1c6c-42f4-9956-6b9d49cdd893

No New Or Fixed Issues Found

bitwarden-bot avatar Feb 22 '24 23:02 bitwarden-bot