Opened on 12/20/2018 at 11:10:11 AM
Closed on 01/14/2019 at 03:54:03 PM
Last modified on 01/17/2019 at 04:02:43 AM
#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 |
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
- Select [ABP Icon] > [Cog] > [Advanced].
- Delete/remove all filter lists.
- 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)
Change History (20)
comment:2 Changed on 01/08/2019 at 02:48:05 PM 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: ↓ 4 Changed on 01/08/2019 at 03:46:39 PM 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 on 01/08/2019 at 10:23:16 PM 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 on 01/09/2019 at 03:02:33 PM 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 on 01/09/2019 at 03:19:33 PM by kzar
comment:6 Changed on 01/09/2019 at 04:46:31 PM 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 on 01/10/2019 at 02:02:18 AM 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 on 01/10/2019 at 11:41:56 AM 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 on 01/12/2019 at 12:02:29 PM by abpbot
A commit referencing this issue has landed:
Issue 7182 - Avoid restoring IndexedDB backup when patterns.ini is empty
comment:10 Changed on 01/12/2019 at 12:04:03 PM 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 on 01/14/2019 at 10:48:59 AM 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 on 01/14/2019 at 12:10:19 PM by kzar
- Review URL(s) modified (diff)
- Status changed from reopened to reviewing
comment:13 Changed on 01/14/2019 at 03:53:40 PM by abpbot
A commit referencing this issue has landed:
Issue 7182 - Fix the Edge IndexedDB unit tests
comment:14 Changed on 01/14/2019 at 03:54:03 PM by kzar
- Resolution set to fixed
- Status changed from reviewing to closed
comment:15 Changed on 01/15/2019 at 01:46:48 PM 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 on 01/15/2019 at 02:57:24 PM 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 on 01/16/2019 at 10:50:59 AM 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 on 01/16/2019 at 12:44:23 PM 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 on 01/17/2019 at 04:02:43 AM by sebastian
- Milestone changed from Adblock-Plus-for-Chrome-Opera-Firefox-next to Adblock-Plus-3.4.3-for-Chrome-Opera-Firefox
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?