Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#2567 closed defect (fixed)

Number of total blocked ads is reset when reloading Adblock Plus

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

http://codereview.adblockplus.org/5968535989780480

Description (last modified by Ross)

Adapted from https://www.passbrains.com/dashboard/view-ticket.php?ticket_no=AOP-222

Environment

Windows + 8 64bit + Opera + English
ABP version 1.8.12.1423

How to reproduce

  1. Install ABP extension.
  2. Load any website for ex: timesofindia.indiatimes.com
  3. Click on ABP icon and check the "total Ads blocked" count.
  4. Right-click on ABP icon and choose "Manage extensions".
  5. Click on "Disable", then "Enable" buttons.
  6. Open a new tab.

Observed behaviour

total Ads blocked count is zero.
operaBefDis,png - shows count before disabling and enabling
operaAftDis.png - shows count after disabling and enabling

Expected behaviour

total Ads blocked count is non-zero.

Attachments (2)

3766_1430126721_operaBefDis.png (754.0 KB) - added by passbrains 5 years ago.
3766_1430126721_operaAftDis.png (404.9 KB) - added by passbrains 5 years ago.

Download all attachments as: .zip

Change History (16)

Changed 5 years ago by passbrains

Changed 5 years ago by passbrains

comment:1 Changed 5 years ago by passbrains

1 - 20 May 2015 22:46:13 posted by Ross Green
Reproduced as described.

The total count resets to 0 on disable/enable but other saved data like preferences persists. On a hunch I installed an older build made before the storage changes (1.8.12.1352) and the total count *is* kept on disable/enable so this seems like a regression.

ABP ABP 1.8.12.1430
Opera 29.0.1795.60 / Windows Vista x64

comment:2 Changed 5 years ago by Ross

  • Component changed from Unknown to Libadblockplus
  • Description modified (diff)

comment:3 Changed 5 years ago by greiner

  • Component changed from Libadblockplus to Platform

I can confirm that this issue is a regression caused by #2040.

comment:4 Changed 5 years ago by sebastian

  • Cc sebastian added
  • Priority changed from Unknown to P2
  • Ready set

Reproduced, on Opera. Chrome doesn't seem to have that issue.

comment:5 Changed 5 years ago by sebastian

  • Summary changed from Total Ads blocked count is resetting to zero when ABP is disabled and enabled. to Number of total ads is reset when reloading Adblock Plus on Opera

comment:6 Changed 5 years ago by sebastian

  • Summary changed from Number of total ads is reset when reloading Adblock Plus on Opera to Number of total blocked ads is reset when reloading Adblock Plus on Opera

comment:7 Changed 5 years ago by sebastian

  • Owner set to sebastian
  • Summary changed from Number of total blocked ads is reset when reloading Adblock Plus on Opera to Number of total blocked ads is reset when reloading Adblock Plus

Now, I can reproduce it on Chrome as well. However, both on Chrome and Opera, the issue doesn't occur when the total number of blocked ads were >0, before updating to >=1.8.12.1417.

The issue is caused by following code path in the preference getter:

if (value == defaultValue)
{
  delete overrides[pref];
  ext.storage.remove(prefToKey(pref));
}

Since the preference stats_total is an object, the check above still evaluates to true, after setting the blocked property in lib/stats.js:

if ("blocked" in Prefs.stats_total)
  Prefs.stats_total.blocked++;
else
  Prefs.stats_total.blocked = 1;
Prefs.stats_total = Prefs.stats_total;

This regression was introduced with #2040, since the values aren't serialized as JSON anymore.

comment:8 follow-up: Changed 5 years ago by sebastian

  • Cc greiner added

@greiner: Any reason you made Prefs.stats_total an object with a single property (ie. blocked), instead simply adding Prefs.total_blocked as integer property? I consider turning it into the latter, as this would make things quite simpler (and also slightly more efficient).

comment:9 in reply to: ↑ 8 Changed 5 years ago by greiner

Replying to sebastian:

@greiner: Any reason you made Prefs.stats_total an object with a single property (ie. blocked), instead simply adding Prefs.total_blocked as integer property? I consider turning it into the latter, as this would make things quite simpler (and also slightly more efficient).

Yes, the initial idea with the ad counter was that it would only be one part in the UI where we display interesting stats and that we'd expose even more as part of the new options page. Therefore this construct was the most flexible to make the storage of various stats more future-proof.

Obviously, a lot has changed in the meantime so we'd need to reassess whether we'd still like to expose more stats or whether we're content with having just the ad counter.

comment:10 Changed 5 years ago by sebastian

  • Review URL(s) modified (diff)

Thanks for the background. If you don't mind, I am going to simplify the data structure for now. It's unlikely that we start storing any other stats in a similar way any time soon. And even then I'd prefer to have a separate preference for each value.

comment:11 Changed 5 years ago by sebastian

  • Status changed from new to reviewing

comment:12 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:13 Changed 5 years ago by sebastian

  • Cc Ross added

@Ross: Note that this fix also touched the migration code of #2040. So it probably makes sense to test the migration again, with particular attention on the total number of blocked ads.

comment:14 Changed 4 years ago by Ross

  • Verified working set

Fixed. Also tested migration from previous builds (1394 > 1433, 1355 > 1433).

ABP 1.8.12.1433
Opera 29.0.1795.60 / Windows Vista x64

Note: See TracTickets for help on using tickets.