Opened 12 months ago

Closed 11 months ago

Last modified 11 months ago

#7182 closed defect (fixed)

First run page is not displayed in Edge after all filter lists have been removed

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

https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/24
https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/27

Description (last modified by kzar)

Environment

ABP 0.9.11.2206 (Devbuild)
Edge 42.17134.1.0 / EdgeHTML 17.17134

IS a regression compared to 0.9.11 (Release)

How to reproduce

  1. Select [ABP Icon] > [Cog] > [Advanced].
  2. Delete/remove all filter lists.
  3. Restart the browser.

Observed behaviour

The user is resubscribed to an EasyList subscription as expected, however the first run page is not displayed with the message explaining what has occured (as happens in Chrome/Firefox).

Expected behaviour

The first run page to be displayed on restart when the user is resubscribed to EasyList because all subsriptions were missing/removed.

Notes

  • This is a regression since the 0.9.11 release.

Attachments (1)

7182-edge-first-run-page.png (264.5 KB) - added by kzar 11 months ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 11 months ago by Ross

  • Description modified (diff)

After double checking, it turns out this is a regression because the previous/current release (0.9.11) does redisplay the first run page with the warning.

Not sure if it's worth delaying the release for though?

comment:2 Changed 11 months ago by kzar

  • Description modified (diff)

Not sure if it's worth delaying the release for though?

I'm not sure, let's see what Sebastian thinks. But in the mean time I'll take a look and see if I can figure out what's going on here.

comment:3 follow-up: Changed 11 months ago by kzar

I'm still waiting for my VM to update (yay Windows 10...), but I wonder if this could be a side effect of the change for #6775?

comment:4 in reply to: ↑ 3 Changed 11 months ago by sebastian

Replying to Ross:

Not sure if it's worth delaying the release for though?

If this is a regression, we should address it before the release.

Replying to kzar:

but I wonder if this could be a side effect of the change for #6775?

I first thought the same, but we only restore filter data from the backup when the file doesn't exist in IndexedDB anymore, but after removing all filter lists the file should still exist at least.

comment:5 Changed 11 months ago by kzar

  • Component changed from Unknown to Platform
  • Owner set to kzar
  • Priority changed from Unknown to P2
  • Ready set

I finally got my Windows 10 VM to cooperate. I can reproduce the problem, investigating now.

Changed 11 months ago by kzar

comment:6 Changed 11 months ago by kzar

I can reproduce this problem perhaps half the time.

Like Sebastian says, the getBackupData() function shouldn't be called at all, since patterns.ini exists. But it seems that after realising that patterns.ini is empty, the filterStorage code attempts to read patterns-backup1.ini, and that in turn triggers the getBackupData() to be called, I guess since that file doesn't exist.

When the first run page is displayed correctly, the getBackupData() function is returning {type: "NoSuchFile"}, but otherwise it's returning the file's contents - which contains the subscriptions, even though I removed them.

Order of events is something like this:

statFile patterns.ini
getFile patterns.ini Result [object Object]
statFile returning...
getFile patterns.ini Result [object Object]
Error: No data in the file
getFile patterns-backup1.ini Result undefined
getBackupData Result {object containing the subscriptions}
First run? false

I'm not sure why the getBackupData() function sometimes returns the subscriptions and sometimes returns "No such file". Perhaps it's when there was a backup made before I removed the subscriptions, but not afterwards? I suppose that timing aspect might explain why I can't reproduce this every time.

I'll continue investigating tomorrow, any suggestions welcome.

comment:7 Changed 11 months ago by sebastian

The backup is deferred by 60 seconds. So if Microsoft Edge is closed within 60 seconds after the subscriptions have been removed, the backup available on next browser start is from before removing the subscriptions. But if you wait for the backup to complete before terminating Microsoft Edge, the backup will be empty as well. Furthermore, there is only one backup in browser.storage.local that is returned for any file that failed to load from IndexedDB.

comment:8 Changed 11 months ago by kzar

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

Gotya thanks, that makes sense.

So if I understand this all right, we can simply skip restoring from our IndexedDB backup if the failed read is for one of these patterns-backupX.ini files. I've opened a review to do that now.

comment:9 Changed 11 months ago by abpbot

comment:10 Changed 11 months ago by kzar

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

(I don't see a Adblock-Plus-For-Edge-next milestone, so hopefully this milestone is OK.)

comment:11 Changed 11 months ago by kzar

  • Resolution fixed deleted
  • Status changed from closed to reopened

Seems my last commit broke the IndexedDB unit tests, looking it that now.

comment:12 Changed 11 months ago by kzar

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

comment:13 Changed 11 months ago by abpbot

A commit referencing this issue has landed:
Issue 7182 - Fix the Edge IndexedDB unit tests

comment:14 Changed 11 months ago by kzar

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

comment:15 Changed 11 months ago by Ross

In the build below, if I removed the lists and close immediately close Edge. On restart the first run page is displayed with the message. If I wait a while (guessing more than 60 seconds) the first run page is not displayed and no lists are added. Not sure if I need to wait for 2221?

ABP 0.9.11.2216 (Devbuild)
Edge 44.17763.1.0 / EdgeHTML 18.17763

comment:16 Changed 11 months ago by geo

I've checked the current build, this fix is not included,so we have to wait for the new build to be published.

comment:17 Changed 11 months ago by kzar

So I installed the devbuild this morning, the store gave me 0.9.11.2223. I tested removing all the subscriptions, and then restarting Edge both immediately and after a few minutes. Everything worked as expected, the first run page always showed up with the message about data corruption.

comment:18 Changed 11 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed. Works for me too both immediately and after a few minutes.

ABP 0.9.11.2223 (Devbuild)
Edge 44.17763.1.0 / EdgeHTML 18.17763

comment:19 Changed 11 months ago by sebastian

  • Milestone changed from Adblock-Plus-for-Chrome-Opera-Firefox-next to Adblock-Plus-3.4.3-for-Chrome-Opera-Firefox
Note: See TracTickets for help on using tickets.