Opened 3 months ago

Closed 3 months ago

#7381 closed change (fixed)

Don't use XMLHttpRequest from the background page

Reported by: kzar Assignee: mjethani
Priority: P2 Milestone:
Module: Core Keywords: manifestv3
Cc: sebastian, mjethani Blocked By:
Blocking: #5702, #7338, #7392, #7406 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/37

Description (last modified by mjethani)

Background

With ​manifest V3, background pages no longer have access to any DOM APIs but are limited to web APIs provided in Service Workers (besides extension APIs). We therefore need to stop assigning XMLHttpRequest.prototype.channel from the adblockpluschrome code, but before we can do that we need to stop using XMLHttpRequest in the adblockpluscore code.

What to change

Replace XMLHttpRequest usage with the fetch API.

Follow-up work

See #7392, #7393.

Hints for testers

Same as #7376.

Additionally, try the following tests:

  1. Use an invalid subscription URL like ht?tp://example.com/subscription.txt. It should give an appropriate error.
  2. Try to sync the subscription while the device is offline. It should give an appropriate error.
  3. Serve the subscription from a server that does not return the Access-Control-Allow-Origin header in the response (see CORS). It should work without problems.
  4. Serve the subscription with the response header Cache-Control: max-age=31536000, sync, then add a new filter to the subscription, sync again. The new filter should work as expected.
  5. Make sure that the server does not receive a Referer header in the request.
  6. Serve the subscription with the response header Set-Cookie: sessionid=acd39ae3f160a8aa; Path=/, sync, then sync again, and make sure the server does not receive a Cookie header in the request.
  7. Use a valid subscription URL where the resource is not available (e.g. https://example.com/non-existent-subscription.txt). It should given an appropriate error.
  8. Try serving subscriptions over both HTTP and HTTPS.
  9. Serve the subscription with a MIME type other than text/plain (e.g. use the response header Content-Type: text/javascript or Content-Type: application/json). It should still work without problems. Any filters in the subscription should work as expected.
  10. Serve a subscription from the URL https://example.com/subscription.txt containing the directive Redirect: https://example.com/other-subscription.txt after [Adblock] at the top. Make sure the two subscriptions have different filters. Also make sure they have different titles (e.g. Test Subscription and Other Test Subscription). After you sync, filters from the other subscription should work as expected but filters from the first subscription should not work at all. The options page should display the title of the other subscription.
  11. Try the preceding test again but this time with more than 5 levels of redirects. It should fail after 5 redirects and give an appropriate error.

Also make sure there are no unexpected errors in the background page console.

The changes here are in a very critical area so do test on all supported platforms thoroughly.

If possible/feasible, also test for #7392 and #7393.

Change History (17)

comment:1 Changed 3 months ago by kzar

  • Priority changed from Unknown to P2
  • Ready set

comment:2 Changed 3 months ago by mjethani

  • Blocked By 7382 added

comment:3 Changed 3 months ago by mjethani

I have filed #7382 specifically for XMLHttpRequest, which is also related to #5702. This can be a meta ticket of sorts to address "XMLHttpRequest and other APIs".

comment:4 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 3 months ago by kzar

I would have rather create the second issue when it turned out to be useful, since so far it's essentially a duplicate of this issue. There might not even be any more things to modify after XMLHttpRequest, and if they are they're unlikely to block #7338.

comment:6 Changed 3 months ago by mjethani

When are we going to know if there are or aren't any more changes to make? I used to keep related changes in the same ticket, but as Thomas pointed out (see ticket:7094#comment:17) it becomes difficult to track changes. It's easier to have exactly one well-defined change per ticket. With #7382 I can say exactly what needs to be changed, and once the change lands we close the ticket (instead of leaving it open).

comment:7 Changed 3 months ago by sebastian

I'm somewhat confident that XMLHttpRequest is the only web API used in adblockpluscore that isn't available in Service Workers (and therefore neither in manifest V3 background pages). Note that the XMLHttpRequest global is explicitly whitelisted in lib/.eslintrc.json (unlike in adblockpluschrome where the env is set to browser). So any other global that only exists in the DOM context would already cause an ESLint error. I agree closing this issue as duplicate of #7382.

comment:8 Changed 3 months ago by kzar

  • Blocked By 7382 removed
  • Description modified (diff)
  • Summary changed from Don't use XMLHttpRequest and other APIs unavailable from ServiceWorkers from the background page to Don't use XMLHttpRequest from the background page

There you go, I've updated this issue to only include the XMLHttpRequest changes and added some further detail about why they block the #7338.

If you notice further changes to adblockpluscore are required for the code to work with the APIs available to Service Workers feel free to file another issue.

comment:9 Changed 3 months ago by kzar

  • Blocking 5702 added

comment:10 Changed 3 months ago by sebastian

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

comment:11 Changed 3 months ago by sebastian

  • Owner set to mjethani

comment:12 Changed 3 months ago by mjethani

  • Blocking 7406 added

comment:13 Changed 3 months ago by kzar

  • Blocking 7392 added

comment:14 Changed 3 months ago by abpbot

A commit referencing this issue has landed:
Issue 7381 - Replace XMLHttpRequest with Fetch

comment:15 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:16 Changed 3 months ago by mjethani

Hopefully I have covered everything in the hints for testers.

comment:17 Changed 3 months ago by mjethani

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.