Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CRX] Migrate Chrome extension to Manifest Version 3 #18681

Merged
merged 9 commits into from
Sep 8, 2024

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented Sep 2, 2024

This PR updates the implementation of the Chrome extension to Manifest Version 3. If you're interested in the individual units of change, review the individual commits and their commit messages.

I manually tested this in Chrome 127, 128 and 130 (and also as old as Chrome 96). Scenarios tested include: http(s) PDFs, link click (verifying that Referer is set), file:-navigations.

Fixes #15161

extensions/chromium/preserve-referer.js Fixed Show fixed Hide fixed
extensions/chromium/preserve-referer.js Dismissed Show dismissed Hide dismissed
web/chromecom.js Dismissed Show dismissed Hide dismissed
@Rob--W Rob--W force-pushed the crx-mv3-migration branch 2 times, most recently from 3ed686c to 86293d4 Compare September 2, 2024 02:42
Copy link
Contributor

@timvandermeij timvandermeij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, with the comments below addressed. I'll have another look at this once that's done before setting the final approval because it's quite a bit of code changes, so I'd like to make sure I haven't missed anything in this round.

extensions/chromium/preserve-referer.js Outdated Show resolved Hide resolved
extensions/chromium/pdfHandler.js Outdated Show resolved Hide resolved
extensions/chromium/pdfHandler.js Outdated Show resolved Hide resolved
extensions/chromium/preserve-referer.js Outdated Show resolved Hide resolved
@Rob--W
Copy link
Member Author

Rob--W commented Sep 8, 2024

I've updated the code + rebased the PR. To more easily tell the real difference, here is the diff:

Output of: git diff 86293d43f 0d9d9debd
diff --git a/extensions/chromium/pdfHandler.js b/extensions/chromium/pdfHandler.js
index 024be2760..0fd71e9f1 100644
--- a/extensions/chromium/pdfHandler.js
+++ b/extensions/chromium/pdfHandler.js
@@ -81,7 +81,8 @@ async function registerPdfRedirectRule() {
       action: ACTION_IGNORE_OTHER_RULES,
     },
     {
-      // Do not redirect for URLs containing pdfjs.action=download.
+      // Redirect local PDF files if isAllowedFileSchemeAccess is true. No-op
+      // otherwise and then handled by webNavigation.onBeforeNavigate below.
       condition: {
         regexFilter: "^file://.*\\.pdf$",
         resourceTypes: ["main_frame", "sub_frame"],
@@ -192,7 +193,8 @@ async function registerPdfRedirectRule() {
     },
   ];
   for (const [i, rule] of addRules.entries()) {
-    rule.id = 15161 + i;
+    // id must be unique and at least 1, but i starts at 0. So add +1.
+    rule.id = i + 1;
     rule.priority = addRules.length - i;
   }
   try {
diff --git a/extensions/chromium/preserve-referer.js b/extensions/chromium/preserve-referer.js
index d4da5b8f2..92c628da3 100644
--- a/extensions/chromium/preserve-referer.js
+++ b/extensions/chromium/preserve-referer.js
@@ -34,7 +34,7 @@ limitations under the License.
 
 // g_referrers[tabId][frameId] = referrer of PDF frame.
 var g_referrers = {};
-var g_referrerTimer = {};
+var g_referrerTimers = {};
 // The background script will eventually suspend after 30 seconds of inactivity.
 // This can be delayed when extension events are firing. To prevent the data
 // from being kept in memory for too long, cap the data duration to 5 minutes.
@@ -46,26 +46,25 @@ var g_postRequests = {};
 var rIsReferer = /^referer$/i;
 chrome.webRequest.onSendHeaders.addListener(
   function saveReferer(details) {
-    if (!g_referrers[details.tabId]) {
-      g_referrers[details.tabId] = {};
-    }
-    g_referrers[details.tabId][details.frameId] = details.requestHeaders.find(
-      h => rIsReferer.test(h.name)
+    const { tabId, frameId, requestHeaders, method } = details;
+    g_referrers[tabId] ??= {};
+    g_referrers[tabId][frameId] = requestHeaders.find(h =>
+      rIsReferer.test(h.name)
     )?.value;
-    setCanRequestBody(details.tabId, details.frameId, details.method !== "GET");
-    forgetReferrerEventually(details.tabId);
+    setCanRequestBody(tabId, frameId, method !== "GET");
+    forgetReferrerEventually(tabId);
   },
   { urls: ["*://*/*"], types: ["main_frame", "sub_frame"] },
   ["requestHeaders", "extraHeaders"]
 );
 
 function forgetReferrerEventually(tabId) {
-  if (g_referrerTimer[tabId]) {
-    clearTimeout(g_referrerTimer[tabId]);
+  if (g_referrerTimers[tabId]) {
+    clearTimeout(g_referrerTimers[tabId]);
   }
-  g_referrerTimer[tabId] = setTimeout(() => {
+  g_referrerTimers[tabId] = setTimeout(() => {
     delete g_referrers[tabId];
-    delete g_referrerTimer[tabId];
+    delete g_referrerTimers[tabId];
     delete g_postRequests[tabId];
   }, REFERRER_IN_MEMORY_TIME);
 }
@@ -75,9 +74,7 @@ function forgetReferrerEventually(tabId) {
 // tracking, it is still here to enable re-use of the webRequest listener above.
 function setCanRequestBody(tabId, frameId, isPOST) {
   if (isPOST) {
-    if (!g_postRequests[tabId]) {
-      g_postRequests[tabId] = new Set();
-    }
+    g_postRequests[tabId] ??= new Set();
     g_postRequests[tabId].add(frameId);
   } else {
     g_postRequests[tabId]?.delete(frameId);

webRequestBlocking is unavailable in MV3. Non-blocking webRequest can
still be used to detect the Referer, but we have to use
declarativeNetRequest to change the Referer header as needed.
MV3 does not support chrome_style in options_ui. Remove it and replace
it with the minimal amount of styles that still has some spacing around
the individual settings for readability.
restoretab.js was added in mozilla#6233
with the purpose of restoring lost tabs when Chrome closes all extension
tabs when it reloads the extension. This forced reload can happen when
the user toggles the "Allow access to file URLs" option.

This logic does not work any more, and since the use of localStorage is
a blocker in migrating to MV3, this patch just drops the logic.
In MV3, the background script is a service worker. localStorage is not
supported in service workers. Switch to storage.local instead.

Data migration is not implemented because it is not needed due to the
privacy-friendly design of the telemetry: In practice the data is reset
about every 4 weeks, when the major version of Chrome is updated.
- Replace DOM-based pdfHandler.html (background page) with background.js
  (extension service worker).

- Adjust logic of background scripts to account for the fact that the
  scripts can execute repeatedly during a browser session. Primarily,
  register relevant extension event handlers at the top level and use
  in-memory storage.session API to keep track of initialization state.

- Extension URL router: replace blocking webRequest with the service
  worker-specific "fetch" event.

- PDF detection: replace blocking webRequest with declarativeNetRequest.
  This requires Chrome 128+. The next commit will add a fallback for
  earlier Chrome versions.
The minimum required version is Chrome 103 because wildcard support for
web_accessible_resources[].extension_id was introduced in 103, in
https://chromium.googlesource.com/chromium/src/+/c9caeb1a080f165f48d2a90559aa35d22965b440

A way to broaden compatibility is to drop that key. By doing so, the
minimum required Chrome version is then 96, because of the
chrome.storage.session API (and declarativeNetRequestWithHostAccess).

Since gulpfile.js already defines "Chrome >= 98" and there is no real
point in expanding support to older versions, I'm just setting the
minimum version to 103.
@Rob--W
Copy link
Member Author

Rob--W commented Sep 8, 2024

Addressed #18681 (comment)

Output of: git diff 0d9d9debd 4bf7be642
diff --git a/extensions/chromium/preserve-referer.js b/extensions/chromium/preserve-referer.js
index 92c628da3..b352dee36 100644
--- a/extensions/chromium/preserve-referer.js
+++ b/extensions/chromium/preserve-referer.js
@@ -112,7 +112,7 @@ chrome.runtime.onConnect.addListener(function onReceivePort(port) {
       referer = data.referer;
     }
     dnrRequestId = data.dnrRequestId;
-    setStickyReferrer(data.requestUrl, () => {
+    setStickyReferrer(dnrRequestId, tabId, data.requestUrl, referer, () => {
       // Acknowledge the message, and include the latest referer for this frame.
       port.postMessage(referer);
     });
@@ -120,41 +120,40 @@ chrome.runtime.onConnect.addListener(function onReceivePort(port) {
 
   // The port is only disconnected when the other end reloads.
   port.onDisconnect.addListener(function () {
-    unsetStickyReferrer();
+    unsetStickyReferrer(dnrRequestId);
   });
-  function setStickyReferrer(url, callback) {
-    if (!referer) {
-      unsetStickyReferrer();
-      callback();
-      return;
-    }
-    const rule = {
-      id: dnrRequestId,
-      condition: {
-        urlFilter: `|${url}|`,
-        // The viewer and background are presumed to have the same origin:
-        initiatorDomains: [location.hostname], // = chrome.runtime.id.
-        resourceTypes: ["xmlhttprequest"],
-        tabIds: [tabId],
-      },
-      action: {
-        type: "modifyHeaders",
-        requestHeaders: [
-          { operation: "set", header: "referer", value: referer },
-        ],
-      },
-    };
-    chrome.declarativeNetRequest.updateSessionRules(
-      { removeRuleIds: [dnrRequestId], addRules: [rule] },
-      callback
-    );
+});
+
+function setStickyReferrer(dnrRequestId, tabId, url, referer, callback) {
+  if (!referer) {
+    unsetStickyReferrer(dnrRequestId);
+    callback();
+    return;
   }
+  const rule = {
+    id: dnrRequestId,
+    condition: {
+      urlFilter: `|${url}|`,
+      // The viewer and background are presumed to have the same origin:
+      initiatorDomains: [location.hostname], // = chrome.runtime.id.
+      resourceTypes: ["xmlhttprequest"],
+      tabIds: [tabId],
+    },
+    action: {
+      type: "modifyHeaders",
+      requestHeaders: [{ operation: "set", header: "referer", value: referer }],
+    },
+  };
+  chrome.declarativeNetRequest.updateSessionRules(
+    { removeRuleIds: [dnrRequestId], addRules: [rule] },
+    callback
+  );
+}
 
-  function unsetStickyReferrer() {
-    if (dnrRequestId) {
-      chrome.declarativeNetRequest.updateSessionRules({
-        removeRuleIds: [dnrRequestId],
-      });
-    }
+function unsetStickyReferrer(dnrRequestId) {
+  if (dnrRequestId) {
+    chrome.declarativeNetRequest.updateSessionRules({
+      removeRuleIds: [dnrRequestId],
+    });
   }
-});
+}

Or perhaps a bit easier to review:

Output of: git diff 0d9d9debd 4bf7be642 --ignore-space-change
diff --git a/extensions/chromium/preserve-referer.js b/extensions/chromium/preserve-referer.js
index 92c628da3..b352dee36 100644
--- a/extensions/chromium/preserve-referer.js
+++ b/extensions/chromium/preserve-referer.js
@@ -112,7 +112,7 @@ chrome.runtime.onConnect.addListener(function onReceivePort(port) {
       referer = data.referer;
     }
     dnrRequestId = data.dnrRequestId;
-    setStickyReferrer(data.requestUrl, () => {
+    setStickyReferrer(dnrRequestId, tabId, data.requestUrl, referer, () => {
       // Acknowledge the message, and include the latest referer for this frame.
       port.postMessage(referer);
     });
@@ -120,11 +120,13 @@ chrome.runtime.onConnect.addListener(function onReceivePort(port) {
 
   // The port is only disconnected when the other end reloads.
   port.onDisconnect.addListener(function () {
-    unsetStickyReferrer();
+    unsetStickyReferrer(dnrRequestId);
   });
-  function setStickyReferrer(url, callback) {
+});
+
+function setStickyReferrer(dnrRequestId, tabId, url, referer, callback) {
   if (!referer) {
-      unsetStickyReferrer();
+    unsetStickyReferrer(dnrRequestId);
     callback();
     return;
   }
@@ -139,22 +141,19 @@ chrome.runtime.onConnect.addListener(function onReceivePort(port) {
     },
     action: {
       type: "modifyHeaders",
-        requestHeaders: [
-          { operation: "set", header: "referer", value: referer },
-        ],
+      requestHeaders: [{ operation: "set", header: "referer", value: referer }],
     },
   };
   chrome.declarativeNetRequest.updateSessionRules(
     { removeRuleIds: [dnrRequestId], addRules: [rule] },
     callback
   );
-  }
+}
 
-  function unsetStickyReferrer() {
+function unsetStickyReferrer(dnrRequestId) {
   if (dnrRequestId) {
     chrome.declarativeNetRequest.updateSessionRules({
       removeRuleIds: [dnrRequestId],
     });
   }
-  }
-});
+}

@timvandermeij timvandermeij merged commit a1b45d6 into mozilla:master Sep 8, 2024
6 checks passed
@timvandermeij
Copy link
Contributor

Nice work; thank you!

@Rob--W Rob--W mentioned this pull request Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plans for MV3?
2 participants