Number of total blocked ads is reset when reloading Adblock Plus

[passbrains]

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.

[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

[greiner]

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

[sebastian]

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

[sebastian]

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.

[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).

[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.

[sebastian]

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.

[sebastian]

​https://hg.adblockplus.org/adblockpluschrome/rev/babe342bc511

[sebastian]

@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.

[Ross]

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

ABP 1.8.12.1433
Opera 29.0.1795.60 / Windows Vista x64