Opened 9 months ago

Closed 9 months ago

Last modified 5 months ago

#7376 closed change (fixed)

Remove references to XMLHttpRequest.channel

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

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

Description (last modified by mjethani)

Background

adblockpluscore still uses internal Gecko APIs (#5702) via wrappers. In practice they do nothing. One of these APIs is XMLHttpRequest.channel. We create a mock version of this API in the WebExt version of Adblock Plus, but this is totally unnecessary now since the legacy extension is no longer supported.

What to change

In lib/downloader.js, stop referring to the channel property of the XMLHttpRequest object. Modify test/_common.js as well as any other test files to stop using this API.

Integration notes

After this change, there's no need for nsIHttpChannel and XMLHttpRequest.prototype.channel in lib/compat.js in adblockpluschrome; these should be removed.

Also, in adblockpluschrome, remove &channelStatus=%CHANNELSTATUS% from defaults.subscriptions_fallbackurl in lib/prefs.js.

Hints for testers

Make sure downloadable subscriptions are synced correctly. You can do this by adding a new filter to a downloadable subscription, manually updating the subscription on the options page, and trying out the new filter. Turn off internet access to the device and try to sync the subscription again, then go online again, add another new filter, sync again, and see that the last added filter works.

Follow-up work

See #7393.

Change History (18)

comment:1 Changed 9 months ago by mjethani

  • Description modified (diff)

comment:2 Changed 9 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 9 months ago by mjethani

  • Owner set to mjethani

comment:4 Changed 9 months ago by mjethani

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

comment:5 Changed 9 months ago by mjethani

  • Blocking 5702 added

comment:6 Changed 9 months ago by mjethani

  • Cc sebastian hfiguiere added
  • Ready set

comment:7 follow-up: Changed 9 months ago by sebastian

Please add integration notes, to remove obsolete code from lib/compat.js in adblockpluschrome.

Also what would you think about using the fetch() API instead while at it?

comment:8 Changed 9 months ago by mjethani

  • Description modified (diff)

comment:9 in reply to: ↑ 7 Changed 9 months ago by mjethani

Replying to sebastian:

Please add integration notes, to remove obsolete code from lib/compat.js in adblockpluschrome.

Done.

Also what would you think about using the fetch() API instead while at it?

I like the idea, but that would require some study of the API to make sure it has the same semantics and fulfills the requirements. We should do it as a next step. In this step we get rid of the Cr namespace.

comment:10 Changed 9 months ago by abpbot

A commit referencing this issue has landed:
Issue 7376 - Remove references to XMLHttpRequest.channel

comment:11 Changed 9 months ago by mjethani

  • Description modified (diff)

comment:12 Changed 9 months ago by mjethani

  • Cc kzar Ross added

comment:13 Changed 9 months ago by mjethani

@sebastian @kzar @Ross I need your help in updating the "Hints for testers" section to add something about the fallback URL. If download fails for 5 attempts (default setting), the extension will consult the fallback URL (which is https://adblockplus.org/getSubscription with some query string parameters by default), but I don't know if there's a way to test this or if tests already exist for this.

comment:14 Changed 9 months ago by mjethani

Also I wonder if the server would have a problem since we are no longer going to be passing the channelStatus query string parameter to the fallback URL (see diff and integration notes).

comment:15 Changed 9 months ago by Ross

I don't think any manual/QA tests exist for this, it's not actually even in the regression sheets. I've made a note to update them to include this. The tester could always just proxy the connection to check the fallback is actually requested (or even point the hostname at a local server).

It doesn't look like channelStatus is used by the server handler? (https://hg.adblockplus.org/sitescripts/file/tip/sitescripts/subscriptions/web/fallback.py)

comment:16 Changed 9 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:17 Changed 9 months ago by mjethani

Since apparently we are not testing the fallback URL feature, I have closed this without any hints for how to test the fallback URL path. I have filed #7393 to properly address that feature. As part of that, we can also write proper tests and make sure the feature is covered in our tests.

comment:18 Changed 5 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described.

ABP 0.9.15.2340
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2340
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.