Opened 3 years ago

Last modified 8 months ago

#5182 new change

Fix IsAllowedConnection — at Version 4

Reported by: sergz Assignee:
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):

Description (last modified by sergz)


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 (4)

comment:1 Changed 3 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, because otherwise tests don't work anymore, and after because it uses that code.

Last edited 3 years ago by sergz (previous) (diff)

comment:2 Changed 3 years ago by sergz

  • Review URL(s) modified (diff)

comment:3 Changed 3 years ago by abpbot

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

comment:4 Changed 3 years 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.

Note: See TracTickets for help on using tickets.