Opened 2 years ago

Last modified 23 months ago

#5182 new change

Fix IsAllowedConnection — at Version 6

Reported by: sergz Assignee: sergz
Priority: P2 Milestone:
Module: Libadblockplus Keywords:
Cc: fhd Blocked By: #5066, #5179, #5198
Blocking: #3595 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29424786/
https://codereview.adblockplus.org/29435650/

Description (last modified by sergz)

Background

IsAllowedConnection had been implemented in a hurry and we had cut some corners, now it's time to fix it, especially because it affects a threading model.

What to change

The issue is pretty comprehensive

  • rename it
    This method is applicable only for subscriptions download, so it should not be called in a so generic manner.
  • remove it from implementation of XMLHttpRequest in compat.js
    However it will be possible only after implementing it in adblockpluscore as #5066.
  • make it asynchronous
    In order to avoid blocking of execution of JS code (see above, it's called from JS and not from a worker thread) the asynchronicity should be either implemented in FilterEngine or supported by a provider of the callback, in the latter case the callback should be async. It seems so far, in order to simplify the life of users of libadblockplus it would be better to have it in FilterEngine.
    Until asynchronous executor is not ready one may just run it a detached thread.

Change History (6)

comment:1 Changed 2 years ago by sergz

  • Blocked By 5198 added

I have removed it from JsEngine and from WebRequestThread and made the execution of it asynchronous in FilterEngine, though it is still in a detached thread (added blocked by #5198).

The codereview will be after landing of https://codereview.adblockplus.org/29424663/, because otherwise tests don't work anymore, and after https://codereview.adblockplus.org/29424659/ because it uses that code.

Last edited 23 months ago by sergz (previous) (diff)

comment:2 Changed 23 months ago by sergz

  • Review URL(s) modified (diff)

comment:3 Changed 23 months ago by abpbot

A commit referencing this issue has landed:
Issue 5182 - fix IsConnectionAllowed

comment:4 Changed 23 months ago by sergz

  • Blocking 5180 removed
  • Description modified (diff)

The landed patch allows to work on #5180 in parallel, so the issue is not blocking #5180 now.

comment:5 Changed 23 months ago by sergz

  • Owner set to sergz

I am working on the part related to #4688, namely removing of LazyWebRequest. It will also cause the change to make IsConnectionAllowed asynchronous. Meanwhile it will be also renamed because the same line will be changed anyway as a part this work.

comment:6 Changed 23 months ago by sergz

  • Review URL(s) modified (diff)
Note: See TracTickets for help on using tickets.