Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#153 closed change (fixed)

Switch to using OS.File for I/O in Firefox

Reported by: trev Assignee: trev
Priority: P3 Milestone: Adblock-Plus-2.6-for-Firefox
Module: Adblock-Plus-for-Firefox Keywords:
Cc: Blocked By: #196, #216, #530
Blocking: #177, #189 Platform:
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/6618710462693376/
http://codereview.adblockplus.org/6726956523454464/
http://codereview.adblockplus.org/6250418359238656/
http://codereview.adblockplus.org/5147885138083840/

Description (last modified by trev)

Background

Currently, the Firefox variant of lib/io.js contains weird hacks to ensure async file I/O. In addition to being very complex, this implementation also isn't completely async - only read and write operations are being performed off the main thread, however file removal, renames and stat operations can still block the main thread execution.

We should drop the hacks in favor of OS.File API which provides async versions of all necessary file operations. Most of it became available with Firefox 18 meaning that we have to drop support for Firefox 17 (current LTS release is Firefox 24).

Change History (17)

comment:1 Changed 5 years ago by trev

  • in_progress set to 1
  • Owner set to trev
  • Ready set
  • Status changed from new to assigned

comment:2 Changed 5 years ago by trev

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

The implementation touched buildtools, adblockplus and adblockplustests repositories, created a review for each of them. The changes are mostly limited to lib/io.js internals, the only changes as far as our API is concerned:

  • IO.readFromFile() can no longer read from a URL (consistent with the Chrome implementation).
  • The decode parameter in IO.readFromFile() and encode parameter in IO.writeToFile() are unused, UTF-8 encoding is used for all files now (consistent with the Chrome implementation).
  • defaults/patterns.ini is no longer being used as fallback if no patterns.ini file is found, instead we set FilterStorage.firstRun flag. This allows removing our current hack in Chrome to detect missing patterns.ini, adjustments will be necessary there however.

comment:3 Changed 5 years ago by trev

Changes to buildtools and adblockplus repositories just landed:

https://hg.adblockplus.org/buildtools/rev/0bbd3ce48940
https://hg.adblockplus.org/adblockplus/rev/3af12903c104

Review for adblockplustests is still outstanding.

comment:4 Changed 5 years ago by trev

  • Blocking 177 added

comment:5 Changed 5 years ago by trev

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:6 Changed 5 years ago by trev

  • Milestone set to Adblock-Plus-for-Firefox-next

comment:7 Changed 5 years ago by trev

Filed #192 for the cleanup that needs to follow.

comment:8 Changed 5 years ago by trev

  • Blocked By 216 added

comment:9 Changed 5 years ago by trev

  • Blocked By 196 added

comment:10 Changed 5 years ago by barbaz

I have a question regarding Gecko compatibility changes due to this issue, and was told to ask here. Why are these changes incompatible with Gecko 18? Latest ABP dev builds seem to work fine on Firefox 18.0.2 here...
(https://adblockplus.org/forum/viewtopic.php?f=1&t=22427)

comment:11 follow-up: Changed 5 years ago by trev

The documentation on OS.File isn't very clear as far as compatibility goes - most of it landed in Gecko 18, some changes were added in Gecko 19 however. I decided to go with Gecko 19 because I didn't want to risk subtle behavior differences breaking something. Why is that important? Anybody should be using Firefox 28 or at the very least Firefox 24 ESR right now, there is absolutely no good reason to use Firefox 18.

comment:12 in reply to: ↑ 11 Changed 5 years ago by barbaz

Replying to trev:

Why is that important? Anybody should be using Firefox 28 or at the very least Firefox 24 ESR right now, there is absolutely no good reason to use Firefox 18.

While you're right that using Firefox 18 is not so smart from a security standpoint, it seems noticeably faster than its predecessors and successors on some machines, and I was just curious whether I could keep running the latest Adblock Plus there. Sounds like I can, but it's completely at my own risk since you're not going to support Gecko 18.

Anyway, thank you for the clear explanation, I'll leave this ticket alone now.

comment:13 Changed 5 years ago by trev

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening: FilterStorage.isFirstRun isn't being set correctly it seems.

comment:14 Changed 5 years ago by trev

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

comment:15 Changed 5 years ago by trev

  • Blocking 189 added

comment:16 Changed 5 years ago by trev

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:17 Changed 5 years ago by trev

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