Opened on 04/26/2016 at 05:55:37 PM

Closed on 04/28/2016 at 12:23:21 PM

Last modified on 10/28/2016 at 07:21:43 AM

#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):

Description (last modified by trev)


Adblock Plus
Firefox 46

How to reproduce

  1. Install BetterPrivacy 1.69 from
  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.


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.

Attachments (0)

Change History (9)

comment:1 Changed on 04/26/2016 at 05:59:32 PM by trev

  • Description modified (diff)

comment:2 Changed on 04/26/2016 at 06:16:33 PM by arthur

  • Cc arthur added

comment:3 Changed on 04/26/2016 at 06:46:28 PM 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 on 04/26/2016 at 06:54:04 PM by trev

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

comment:5 Changed on 04/28/2016 at 12:07:33 PM by abpbot

A commit referencing this issue has landed:

comment:6 Changed on 04/28/2016 at 12:22:16 PM by trev

  • Description modified (diff)

comment:7 Changed on 04/28/2016 at 12:22:40 PM by abpbot

A commit referencing this issue has landed:

comment:8 Changed on 04/28/2016 at 12:23:21 PM by trev

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

comment:9 Changed on 10/28/2016 at 07:21:43 AM by trev

  • Blocked By 4576 added

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from trev.
Note: See TracTickets for help on using tickets.