Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#3993 closed defect (fixed)

Utils.yield() is a footgun

Reported by: trev Assignee: trev
Priority: P2 Milestone: Adblock-Plus-2.8-for-Firefox
Module: Adblock-Plus-for-Firefox Keywords:
Cc: arthur Blocked By: #4576
Blocking: Platform: Firefox
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29340849/
https://codereview.adblockplus.org/29340852/

Description (last modified by trev)

Environment

Adblock Plus 2.7.2.4168
Firefox 46

How to reproduce

  1. Install BetterPrivacy 1.69 from https://addons.mozilla.org//addon/betterprivacy/
  2. Restart Firefox (might need to restart multiple times in order to reproduce the issue).
  3. Open Adblock Plus preferences.

Observed behaviour

Adblock Plus filters aren't loaded.

Expected behaviour

Adblock Plus filters should be loaded.

Background

BetterPrivacy 1.69 is broken in Firefox 46, one of their JavaScript files produces a syntax error (outdated syntax, no longer supported). We call Utils.yield() occasionally while reading patterns.ini, this way we prevent hanging Firefox UI. However, this function will currently spin its own event loop. If we are out of luck, an event processed in our event loop will load BetterPrivacy's JavaScript file and error out. The error is then propagated all the way down to our code which assumes that loading patterns.ini produced an error and falls back to the backup.

What to change

IO.readFromFile() is already async, the progress listener is being called in a synchronous fashion however. One option would be evaluating listener result: yield listener(line). This allows the listener to return a promise that will be resolved asynchronously:

return new Promise((resolve, reject) => Utils.runAsync(resolve));

This approach doesn't have any problematic side-effects, our code is no longer on stack when the error happens.

Hints for testers

The dependency update here also imported the changes from #4000 which could potentially impact CSS property filters. However, without any of the adjustments from #3969 the behavior simply stays unchanged.

Change History (9)

comment:1 Changed 4 years ago by trev

  • Description modified (diff)

comment:2 Changed 4 years ago by arthur

  • Cc arthur added

comment:3 Changed 4 years ago by trev

  • Component changed from Core to Adblock-Plus-for-Firefox
  • Owner set to trev

Moving to Adblock Plus for Firefox module, the Core part here is tiny.

comment:4 Changed 4 years ago by trev

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

comment:5 Changed 4 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/adblockpluscore/rev/3f1bdb2a4fd4

comment:6 Changed 4 years ago by trev

  • Description modified (diff)

comment:7 Changed 4 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/adblockplus/rev/3d6cba98fba5

comment:8 Changed 4 years ago by trev

  • Milestone set to Adblock-Plus-for-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:9 Changed 3 years ago by trev

  • Blocked By 4576 added
Note: See TracTickets for help on using tickets.