Opened 8 months ago

Closed 6 months ago

Last modified 2 weeks ago

#7338 closed change (fixed)

Don't use any DOM APIs in the background page

Reported by: sebastian Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-for-Chrome-Opera-Firefox-next
Module: Platform Keywords: manifestv3
Cc: kzar, greiner Blocked By: #7371, #7381, #7406, #7408, #7423
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/53
https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/54

Description (last modified by kzar)

Background

With manifest V3, background pages no longer have access to any DOM APIs but are limited to the same web APIs provided in Service Workers (besides the extension APIs). Hence, in the future Chrome extensions that attempt to access the global window or document object, or any of their properties imported in the global scope that don't exist in the Service Worker context will fail to run.

What to change

When referring to the global Object self can be used instead of window.

Some APIs we use still exist for Service Workers, but not under the window Object. Alter references to those, to not include window. Specifically window.Notification, window.clearInterval, window.navigator, window.setInterval and window.setTimeout.

Some APIs we use won't be accessible under Service Workers, remove/replace use of those. Specifically:

  • In lib/subscriptionInit.js we use DOMParser to parse subscriptions.xml. We plan to switch to JSON with #7371.
  • In lib/compat.js we configure XMLHttpRequest, for later use in the core code. We plan to switch to the fetch API in the core code with #7381, then we won't need to configure XMLHttpRequest any more.
  • In lib/utils.js we use document.addEventListener, document.removeEventListener and document.readyState in order to provide Utils.runAsync which is used by the core code. Once we stop using runAsync with #7406, we can remove this code.
  • In lib/notificationHelper.js we use window.confirm as a fall back for question notifications, where the browser.notifications API is faulty/unavailable. We plan to remove this fallback with #7408.
  • In lib/filterValidation.js we use document.createElement, document.documentElement.appendChild, document.documentElement.removeChild and document.querySelector to create a style element which we use to test if a given CSS selector is valid. We're going to remove the filterValidation module, once we have stopped using it with #7423.
  • In lib/icon.js we use Image to load the icon images, then document.createElement to create a canvas with their contents as an optimisation (#7253). We'll need to use a combination of the fetch and OffscreenCanvas APIs instead, also providing a polyfill using a canvas element for browsers which don't yet support OffscreenCanvas.

Notes

To confirm which APIs might not exist in Service Workers you can change the ESLint environment from browser to serviceworker for our scripts which run in the background page. But note, I found false positives were flagged, specifically: URL, console.error, console.trace, fetch and indexedDB.

Hints for testers

  • Check that notifications work correctly on all the supported platforms, including old versions (e.g. 49) of Chrome and Edge. Note that we no longer expect question notifications to be shown on Edge.
  • Check that all the icon functionality works correctly, for example that the icon is shown, that it is greyed out for whitelisted pages and that an animation is shown for appropriate notifications. Again, take special care to test with Chrome 49 and Edge as well as the other platforms.
  • Check that $sitekey whitelisting still works correctly.

Change History (34)

comment:1 Changed 8 months ago by kzar

  • Owner set to kzar

comment:2 Changed 8 months ago by kzar

  • Keywords manifestv3 added

comment:3 Changed 7 months ago by kzar

  • Description modified (diff)
  • Summary changed from Don't use any DOM API's in the background page to Don't use any DOM APIs in the background page

comment:4 Changed 7 months ago by kzar

  • Blocked By 7381 added

comment:5 Changed 7 months ago by sebastian

For reference, DOMParser is used to parse subscriptions.xml, but we plan to switch to JSON with #7371.

comment:6 Changed 7 months ago by kzar

  • Blocked By 7371 added
  • Description modified (diff)

comment:7 Changed 7 months ago by kzar

  • Blocked By 7406 added

comment:8 Changed 7 months ago by kzar

  • Description modified (diff)

comment:9 Changed 7 months ago by kzar

  • Description modified (diff)

comment:10 Changed 7 months ago by kzar

  • Description modified (diff)

comment:11 Changed 7 months ago by kzar

  • Description modified (diff)

comment:12 Changed 7 months ago by kzar

  • Description modified (diff)

comment:13 Changed 7 months ago by kzar

  • Blocked By 7408 added

comment:14 Changed 7 months ago by kzar

  • Description modified (diff)

comment:15 Changed 7 months ago by kzar

  • Description modified (diff)

comment:16 Changed 7 months ago by kzar

  • Description modified (diff)

comment:17 in reply to: ↑ description Changed 7 months ago by sebastian

  • In lib/icon.js we use Image to load the icon images, then document.createElement to create a canvas with their contents as an optimisation (#7253). We'll need to pass the image paths to the browserAction APIs instead.

That would not only revert the optimization implemented in #7253, but also break the icon animation. It might be possible to load the image with the fetch() API and then use an OffscreenCanvas instead.

comment:18 Changed 7 months ago by kzar

  • Description modified (diff)

Cool, I just tried that out and it seems possible on Chrome 72 at least. My test code (of course you need to add the eyeo-32.png file first):

let canvas = new OffscreenCanvas(32, 32);
let context = canvas.getContext("2d");
fetch("icons/eyeo-32.png").then(response =>
{
  return response.body.getReader().read();
}).then(responseData =>
{
  return createImageBitmap(new Blob([responseData.value]));
}).then(imageBitmap =>
{
  canvas.globalAlpha = 1;
  context.drawImage(imageBitmap, 0, 0);

  return browser.browserAction.setIcon({
    imageData: {32: context.getImageData(0, 0, 32, 32)}
  });
}).then(() =>
{
  console.log("Icon set");
});

If that turns out to work OK in practice with the real code, I wonder what we should do for browsers that don't yet support OffscreenCanvas, like older versions of Chrome? I guess we can fallback to just using image paths, breaking animations like we used to do for Safari.

comment:19 Changed 7 months ago by sebastian

We can polyfill OffscreenCanvas using a <canvas> DOM element. Old browsers that don't have OffscreenCanvas will still have the DOM in background pages.

comment:20 in reply to: ↑ description Changed 7 months ago by sebastian

  • Cc greiner added
  • In lib/filterValidation.js we use document.createElement, document.documentElement.appendChild, document.documentElement.removeChild and document.querySelector to create a style element which we use to test if a given CSS selector is valid. No plan so far, we'll need to attempt to validate CSS selectors ourselves - perhaps with a big regexp.

Let's just remove the filterValidation module. If we want to keep validating CSS selectors used in element hiding filters, this needs to be done on the content-side (e.g. in the options page) where we can still access the DOM, but that is up to the UI team. However, with that logic removed, all is left of the filterValidation module would be a trivial wrapper around Filter.fromText() and instanceof InvalidFilter. There is no reason for adblockplusui to rely on another module just for that.

Last edited 7 months ago by sebastian (previous) (diff)

comment:21 Changed 7 months ago by kzar

  • Blocked By 7423 added

comment:22 Changed 7 months ago by kzar

  • Description modified (diff)

Alright, that works for me. We have a plan!

comment:23 Changed 7 months ago by kzar

  • Description modified (diff)

comment:24 Changed 7 months ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:25 Changed 7 months ago by kzar

  • Description modified (diff)

comment:26 Changed 7 months ago by kzar

  • Description modified (diff)

comment:28 Changed 7 months ago by kzar

  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next

comment:29 Changed 6 months ago by sebastian

  • Resolution set to fixed
  • Status changed from reviewing to closed

The remaining changes are being addressed as part of chrome#9.

comment:30 Changed 6 months ago by yoyo888

spam

Last edited 2 weeks ago by kzar (previous) (diff)

comment:31 Changed 6 months ago by Monisha G

spam

Last edited 2 weeks ago by kzar (previous) (diff)

comment:32 Changed 5 months ago by tonnyken

spam

Last edited 2 weeks ago by kzar (previous) (diff)

comment:33 Changed 3 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Does not look to have caused any regressions and the functionality mentioned in the testing notes still works as expected.

ABP 0.9.15.2339
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2339
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2

comment:34 Changed 3 months ago by Malcolm37

spam

Last edited 2 weeks ago by kzar (previous) (diff)
Note: See TracTickets for help on using tickets.