Opened 19 months ago

Closed 18 months ago

Last modified 14 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: Ross Verified working: yes
Review URL(s):

Description (last modified by mjethani)


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 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:// 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. 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 containing the directive Redirect: 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 (18)

comment:1 Changed 19 months ago by kzar

  • Priority changed from Unknown to P2
  • Ready set

comment:2 Changed 19 months ago by mjethani

  • Blocked By 7382 added

comment:3 Changed 19 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 19 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 19 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 19 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 19 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 19 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 19 months ago by kzar

  • Blocking 5702 added

comment:10 Changed 19 months ago by sebastian

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

comment:11 Changed 19 months ago by sebastian

  • Owner set to mjethani

comment:12 Changed 18 months ago by mjethani

  • Blocking 7406 added

comment:13 Changed 18 months ago by kzar

  • Blocking 7392 added

comment:14 Changed 18 months ago by abpbot

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

comment:15 Changed 18 months ago by mjethani

  • Description modified (diff)

comment:16 Changed 18 months ago by mjethani

Hopefully I have covered everything in the hints for testers.

comment:17 Changed 18 months ago by mjethani

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

comment:18 Changed 14 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described.

Everything in the hints for testers section looks to be working correctly.

I haven't tested the other mentioned tickets (like #7392) yet but I think that needs some setup and shouldn't block this ticket.

Microsoft Edge 44.17763.1.0 / Windows 10 1809

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

Note: See TracTickets for help on using tickets.