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): |
https://codereview.adblockplus.org/29340849/ |
Description (last modified by trev)
Environment
Adblock Plus 2.7.2.4168
Firefox 46
How to reproduce
- Install BetterPrivacy 1.69 from https://addons.mozilla.org//addon/betterprivacy/
- Restart Firefox (might need to restart multiple times in order to reproduce the issue).
- 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.
Attachments (0)
Change History (9)
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
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:
https://hg.adblockplus.org/adblockpluscore/rev/3f1bdb2a4fd4
comment:7 Changed on 04/28/2016 at 12:22:40 PM by abpbot
A commit referencing this issue has landed:
https://hg.adblockplus.org/adblockplus/rev/3d6cba98fba5
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
Moving to Adblock Plus for Firefox module, the Core part here is tiny.