Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#2658 closed change (fixed)

Get rid of try..catch for filter validation

Reported by: sebastian Assignee: sebastian
Priority: P4 Milestone: Adblock-Plus-1.9-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: Blocked By:
Blocking: Platform: Unknown
Ready: yes Confidential: no
Tester: Verified working: no
Review URL(s):

http://codereview.adblockplus.org/5655474749833216

Description (last modified by sebastian)

Background

The parseFilter() and parseFilters() functions parse filters and raise an exception when encountering invalid filters. Hence calls needs to be wrapped in try..catch blocks. However, the V8 JavaScript engine cannot optimize functions using try statements.

While validating filters isn't really a hotspot, the onMessage handler which currently isn't optimized because of this is one. Removing the try statement there speeds up the time spend in that function by 30%, which corresponds to ~10ms when loading websites like nytimes.com on my notebook.

As a sight effect, returning the error instead throwing it, also speeds up filter validation by ~20%.
Plus it slightly simplifies the calling code, by not having to catch an exception.

What to change

Make parseFilter() and parseFilters() not throw an exception but return an object holding the parsed filter(s) in one property or the error message in another property, and adjust the calling code.

So parseFilters() will either return an object like {filters: [..]} or {error: ..}. And the calling code would look like:

var result = parseFilters(text);
if (result.error)
  ...
else
{
  for (var filter of result.filters)
    ...
}

Hints for testers

Make sure that filter validation works as it did before:

  1. Go to the options page
      1. Add a valid blocking filter (e.g. ||example.com)
      2. Attempt to add an invalid blocking filter (e.g. ||example.com$bad)
      3. Add a valid element hiding filter (e.g. ##.foobar)
      4. Attempt to add an invalid element hiding filter (e.g. ##[foo)
      1. Click "Edit filters as raw text"
      2. Click "Apply changes"
      3. Click "Edit filters as raw text" again
      4. Add some invalid filters (see above for examples)
      5. Click "Apply changes"
  2. Go to any website
    1. Click the ABP icon
    2. Choose "Block Element"
    3. Select any element on the page
    4. Click "Add filters"
    5. Repeat 2.1 through 2.3
    6. Modify the suggested filters producing one or more invalid filters
    7. Click "Add filters"

When you did enter any invalid filters you should see an alert() dialog with an error message. Otherwise the filter(s) should be saved.

Change History (8)

comment:1 Changed 5 years ago by sebastian

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

comment:2 Changed 5 years ago by sebastian

  • Description modified (diff)

comment:3 Changed 5 years ago by sebastian

  • Description modified (diff)

comment:4 Changed 5 years ago by sebastian

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

comment:5 Changed 5 years ago by sebastian

  • Description modified (diff)

comment:6 Changed 5 years ago by sebastian

  • Description modified (diff)

comment:7 Changed 5 years ago by sebastian

  • Description modified (diff)

comment:8 Changed 5 years ago by sebastian

  • Description modified (diff)
Note: See TracTickets for help on using tickets.