Opened 4 years ago

Closed 4 years ago

#2757 closed defect (fixed)

Notification data are reset when pages load during extension intitilization

Reported by: sebastian Assignee: sebastian
Priority: P1 Milestone: Adblock-Plus-2.6.10-for-Firefox
Module: Core Keywords:
Cc: trev, Kirill Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29321425

Description

Environment

Chrome and Opera (any version)
Adblock Plus 1.9 and any devbuild since 1.8.12.1417

How to reproduce

  1. Go to the browser settings

Then either ...

  1. Set "On startup" to either "Continue where you left off"
  2. Open some web pages (3 or more to reproduce reliably)

... or ...

  1. Set "On startup" to either "Open a specific page or set of pages."
  2. Set some start pages (3 or more to reproduce reliably)
  1. Restart the browser

Observed behaviour

The notification data are reset as you can see by typing Prefs.notificationdata in the JavaScript console on the the extension's options or background page. Also therefore the lastVersion and downloadCount parameters are reset to 0 on the next notification request, as you can see in the Networks panel when inspecting the background page, or when looking at the server log.

Expected behaviour

The notification data should always persist across browser restarts.

Background

That is because Notification.getNextToShow() (or Notification._getNextToShow() in more recent revisions), which is called every time a document or subframe loads, sets Prefs.notificationdata.shown to [] (empty array) if that property doesn't exist yet. This didn't cause issues in older version of Adblock Plus for Chrome or on other platforms, because we use synchronous APIs to read the preferences, eliminating timing issues, there. But since we moved from localStorage to the asynchronous chrome.storage API (#2040), the extension might observe new loading pages before the preferences have been read, overriding the notification data from the previous run.

What to change

There is no reason to change Prefs.notificationdata in Notification._getNextToShow(). Instead the code should simply handle Prefs.notificationdata.shown being missing without setting that property.

Change History (2)

comment:1 Changed 4 years ago by sebastian

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

comment:2 Changed 4 years ago by sebastian

  • Milestone set to Adblock-Plus-for-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.