Detect data corruption of storage.local

[sbell]

Background

After releasing Adblock Plus 3.0.3 for Chrome, some users complained that the update (or first-run) page kept showing every time they start the browser, which might also be the reason for the increase in uninstalls.

It turned out that there is a chance that the underlying storage gets corrupted, causing attempts to read from and/or write to storage.local to fail. Since this isn't currently handled, among other issues, this can cause the update page (if only writing fails) or first run page (if reading and writing fails) to show every time.

The best we can do in this case is showing a targeted message to the user, explaining the issue and instructing them to reinstall Adblock Plus.

What to change

Detect if either/both storage.local.get() and storage.local.set() fail. In that case show the first run page with a targeted message, and add dataCorrupted=1 corrupted=1 (see #6655) to the query string of the uninstall page.

Hints for testers

Ideally, you would reproduce the ​actual data corruption inside your browser profile. However, if that is too tricky, you can use ​this patch in order to emulate the behavior of all write operations to the storage failing, or ​this patch emulating read and write operations failing.

[canwuna]

Also happening on Windows 10 PC, Chrome Version 65.0.3325.181 (Official Build) (64-bit)
and Windows 7 PC, Chrome (no info on version)

[greiner]

I wasn't able to reproduce this issue with the following environments:

• Ubuntu 16.04, Chrome 65
• Windows 10, Chrome 63

So far the only thing I found that could lead to this scenario is if, for whatever reason, the ​last_updates_page_displayed preference fails to be updated.

[agiammarchi]

I second Thomas here. Unable to reproduce in Win 10 Home 64 bit and Chrome 66 and I also think, after investigating a bit, that's the most obvious point of failure.

[sebastian]

I also couldn't reproduce it on any of my devices (Debian Linux, Windows 10 and ChromeOS).

@Ross, can you give it a try?

[agiammarchi]

FWIW I've tried to mess up as much as I could the storage and yet, every single time, it worked as expected. With value, wrong value, no value, dirty value, etcetera ... absolutely unable to reproduce on Win 10 Home

[kzar]

Update page popup for Firefox extension on Android

[kzar]

I use Firefox (60.0) on Android (7.0) with the Adblock Plus extension installed on my phone (Moto G4 Plus) and this morning I saw the updates page popup! Needless to say it didn't look great, screenshot attached.

Edit: Ah, sorry this is more relevant to #6604.

[agiammarchi]

That's bad because AFAIK we never had target screen sizes to test that page against.
However, if we end up removing such update page, maybe there's nothing to do in terms of layout.

[philll]

We started asking users for some info to help us debug this:

Hi there,

it's in fact a bug, the page should only have displayed once right after the Adblock Plus update. To help us find the cause, could we ask you to get us some more info by doing the following?

1. Go to chrome://extensions/
2. In top right corner, switch on "Developer mode"
3. At the Adblock Plus extension, click "background page"
4. In the Console tab, paste this and press Enter:
browser.storage.local.get("pref:last_updates_page_displayed").then(console.log)
5. Copy the displayed output here

I sent this to a few via email from support@ and posted in some forum threads:
​https://adblockplus.org/forum/viewtopic.php?f=10&t=55375&p=175102#p175102
​https://adblockplus.org/forum/viewtopic.php?f=10&t=55377

[sebastian]

For reference, there has been some discussion on IRC today, whether we should just remove the update page for now (this is what the current patch under review is about), and if so whether to have an emergency release for this.

We still don't know what is causing this, but if this issue is real, it likely has larger implications beyond the update page. However, we know that with Adblock Plus 3.0.3 the uninstall rate was initially much higher than usual (but meanwhile dropped almost back to normal). But we don't know if / how much this were/is due to this issue, or how many users are effected.

Until we are able to reproduce and properly address this issue (if we ever will), we have following options:

1. Remove the update page with an emergency release (3.0.5)
2. Remove the update page with the next regular release (3.1)
3. Don't do anything until we properly understand the issue.

@fhd: What do you think?

[agiammarchi]

Just a reminder that once we've removed the update page, in a way or another, our extension update would be silent / less obtrusive so that we shouldn't bother users even with future emergencies releases.

If we keep having people complaining and the uninstall rate is still high, I'd personally go for 1 followed by 2 taking time to clean up things properly and/or rethink the whole mechanism to show such page.

[sebastian]

Well, the update page isn't shown for every update. But only for users updating from any version before 3.0.3 to any newer version (until we update the update page to announce a specific future release). Of course users that mistakenly keep seeing the update page are a different story.

As for the current uninstall rate, according to Hub Issue 8749, it meanwhile dropped down to 0.1% (it was 0.07% before this release).

[greiner]

We just received the following error log from a user which confirms our suspicion that it's a data corruption issue that affects multiple areas of the extension:

_generated_background_page.html:1 Unchecked runtime.lastError while running storage.get: Corruption: not an sstable (bad magic number)
    at StorageArea.value [as get] (chrome-extension://cfhdojbkjhnklbpkdaibdccddilifddb/polyfill.js:92:23)
    at Object.get (chrome-extension://cfhdojbkjhnklbpkdaibdccddilifddb/ext/background.js:674:29)
    at resolve (chrome-extension://cfhdojbkjhnklbpkdaibdccddilifddb/lib/adblockplus.js:1530:17)
    at init (chrome-extension://cfhdojbkjhnklbpkdaibdccddilifddb/lib/adblockplus.js:1528:21)
    at Object.platformVersion (chrome-extension://cfhdojbkjhnklbpkdaibdccddilifddb/lib/adblockplus.js:1587:1)
    at __webpack_require__ (chrome-extension://cfhdojbkjhnklbpkdaibdccddilifddb/lib/adblockplus.js:20:30)

polyfill.js:120 Uncaught (in promise) Error: Corruption: not an sstable (bad magic number)
    at StorageArea.value [as set] (polyfill.js:89)
    at Object.set (background.js:680)
    at savePref (prefs.js:264)
    at Object.set [as currentVersion] (prefs.js:318)
    at detectFirstRun (subscriptionInit.js:53)

polyfill.js:120 Uncaught (in promise) Error: Corruption: not an sstable (bad magic number)
    at StorageArea.value [as set] (polyfill.js:89)
    at Object.set (background.js:680)
    at savePref (prefs.js:264)
    at Object.set [as last_updates_page_displayed] (prefs.js:318)
    at finishInitialization (subscriptionInit.js:230)

polyfill.js:120 Uncaught (in promise) Error: The frame was removed.
    at Object.value [as insertCSS] (polyfill.js:89)
    at addStyleSheet (cssInjection.js:73)
    at updateFrameStyles (cssInjection.js:127)
    at port.on (cssInjection.js:164)
    at Port._onMessage (messaging.js:44)
    at ext._EventTarget._dispatch (common.js:41)
    at browser.runtime.onMessage.addListener (background.js:663)
    at wrapper (polyfill.js:153)
    at EventImpl.dispatchToListener (extensions::event_bindings:403)
    at Event.publicClassPrototype.(anonymous function) [as dispatchToListener] (extensions::utils:138:26)

Unchecked runtime.lastError while running storage.set: Corruption: not an sstable (bad magic number)
    at StorageArea.value [as set] (<URL>)
    at Object.set (<URL>)
    at Promise (<URL>)
    at saveFile (<URL>)
    at Object.writeToFile (<URL>)
    at Promise.resolve.then.then.catch.then (<URL>)

polyfill.js:120 Uncaught (in promise) Error: Corruption: not an sstable (bad magic number)
    at StorageArea.value [as set] (polyfill.js:89)
    at Object.set (background.js:680)
    at savePref (prefs.js:264)
    at Object.set [as notificationdata] (prefs.js:318)
    at saveNotificationData (notification.js:55)
    at Object._onExpirationChange (notification.js:160)
    at exports.Downloader._doCheck (downloader.js:150)
    at FakeTimer._timer.initWithCallback [as callback] (downloader.js:49)
    at window.setTimeout (compat.js:108)

polyfill.js:120 Uncaught (in promise) Error: Corruption: not an sstable (bad magic number)
    at StorageArea.value [as set] (polyfill.js:89)
    at Object.set (background.js:680)
    at savePref (prefs.js:264)
    at Object.set [as notificationdata] (prefs.js:318)
    at saveNotificationData (notification.js:55)
    at Object._onDownloadSuccess (notification.js:194)
    at XMLHttpRequest.request.addEventListener.event (downloader.js:324)

Based on that I'd suggest to (a) detect the data corruption and (b) show the first-run page with the existing or a new message to alert users who experience this issue about it.

[agiammarchi]

FWIW, I would also reconsider Prefs proxy to give us a way to eventually intercept these situations.

I am pretty sure the storage.set here would've triggered an error and we could have preventively react somehow to this situation.

As example, showing the update page only after the setting has been successfully stored, instead of assuming a storage save would've always worked, could have made this issue still critical but not annoying for our users.

This is similar to the pattern I've mentioned in IRC. What we do now, metaphorically speaking, is this:

if (doIt) {
  doStuff();
  doIt = false;
}

Instead, we should always go for this pattern

if (doIt) {
  doIt = false;
  doStuff();
}

However, the current sync-like accessor, with async operations behind the scene, doesn't give us a way to read or write preferences through a natural async flow.

// example of flow through async storages
if (itShouldShowUpdate) {
  storage.set(update, 1).then(showUpdatePage);
}

So, maybe to consider for the future, here a basic proof of concept of how our Prefs proxy could look instead:

const Prefs = new Proxy(
  Object.create(null),
  {
    get(target, name) {
      name = 'pref:' + name;
      return target[name] ||
            (target[name] = function (value) {
              const local = browser.storage.local;
              return arguments.length === 0 ?
                      local.get(name).then(result => result[name]) :
                      local.set({[name]: value}).then(() => value);
            });
    }
  }
);

That would give us the ability to Prefs.any_value(val).then(goOn, doSomethingToFixIt)

Behind the scene we also could have a queue based approach that never tries to access the storage concurrently, and it always grants one operation per time.

I also understand the Promise approach of the storage would probably not effectively grant 100% reliable results, but making it less busy, queueing as example all set/get operations, and having a way to fix errors and try to continue the queue of saves, might give us better safe-guards around this storage.

Yet it would be awesome to understand in which circumstances the local storage could fail, 'cause it looks like it's extremely fragile across different browsers.

[sebastian]

Well, if the storage is corrupted there will be a wide range of issues (besides the update page being shown repeatedly currently), and we cannot make Adblock Plus work correctly anyway. So rather than adding complexity to handle asynchronous results ever single time we touch the storage, we might rather want to detect the data corruption once and show a targeted message.

[kzar]

Replying to sebastian:

Well, if the storage is corrupted there will be a wide range of issues (besides the update page being shown repeatedly currently), and we cannot make Adblock Plus work correctly anyway. So rather than adding complexity to handle asynchronous results ever single time we touch the storage, we might rather want to detect the data corruption once and show a targeted message.

Yea, looking at those exceptions perhaps we could add try ... catch ... inside ext.storage get, set and remove. If we caught an exception we could set a corrupted flag, or display some kind of message to the user.

I'm looking into the problem itself, to see if I can find any other people having these data corruption problems or if there's anything else we can do for those users.

[fhd]

Replying to sebastian:

@fhd: What do you think?

I may be missing something, but I don't see a strong reason for doing anything rash, i.e. your option (1). Do you (or somebody else) see one?

Option (2) is fine by me, no strong opinion. Most people have presumably already seen the update page anyway.

[sebastian]

Replying to kzar:

Yea, looking at those exceptions perhaps we could add try ... catch ... inside ext.storage get, set and remove. If we caught an exception we could set a corrupted flag, or display some kind of message to the user.

As far as I understand, ​this is how we'd have to detect the data corruption, if we want to show a targeted message on the first run page.

Replying to fhd:

I may be missing something, but I don't see a strong reason for doing anything rash, i.e. your option (1). Do you (or somebody else) see one?

Option (2) is fine by me, no strong opinion. Most people have presumably already seen the update page anyway.

Well, things changed a little by now, since we now have seen the underlying data corruption, and it might make more sense to show a targeted message to effected users. Whether to remove the update page in addition, no strong opinion either. But FWIW, I'd also rather avoid rushing another emergency release.

[agiammarchi]

Replying to sebastian:

Well, if the storage is corrupted there will be a wide range of issues

like I've said, the issue is still critical, but the update page wouldn't be shown. My suggestion is about delivering better UX, not to avoiding corrupted data since it might happen regardless.

rather than adding complexity to handle asynchronous results

It's a logical, more robust, async flow, based on Promises, which are established primitives of the language.

I don't see any extra complexity having a Proxy based on promises, but I do see lack of control over a fake synchronous operation that doesn't grant any result, not even through a try/catch.

With the proposed code instead, we wouldn't at least bother users, keeping the ability to investigate the underlying issue or react in core.

Again, it's nothing urgent, but it'd be cool to consider it as an improvement in the future. It will also play well with await once available cross targets.

Replying to fhd:
I may be missing something, but I don't see a strong reason for doing anything rash, i.e. your option (1). Do you (or somebody else) see one?

Agreed with Sebastian, we have now a better understanding of the issue and dropping only the update page wouldn't solve much.

However, if we are not planning to show such page any time soon in the future, the related code review got an LGTM already and it's always good to drop some extra code we most likely don't need.

[kzar]

Replying to agiammarchi:

... the related code review got an LGTM already ...

To clarify it didn't get a LGTM yet, it got a "Looks good to me" which to me means something like "The changes so far look OK in themselves, but there's a reason why we shouldn't land it yet".

[philll]

The user we received the console log from tried clearing his local storage via console and got this:

browser.storage.local.clear()
undefined

Unchecked runtime.lastError while running                                          _generated_background_page.html:1  
storage.clear: Corruption: not an sstable (bad magic number)

He said that "Upon restarting Chrome, the same condition- the update page came up."

[abpbot]

A commit referencing this issue has landed:
​Issue 6599 - Added data corruption warning to first-run page

[abpbot]

A commit referencing this issue has landed:
​Issue 6599 - Detect data corruption of storage.local

[Ross]

Done. If local storage is corrupted the first run page is displayed with the message asking users to reinstall. No side-effects in Firefox. The corrupted flag on the uninstall page is correct. Checked first run page + message also appears if upgrading from 3.0.3 and had already seen the first run page.

ABP 3.0.4.2042
Firefox 59 / 55 / 51 / Windows 10
Chrome 66 / 58 / 49 / Windows 7
Opera 52 / 45 / 36 / Windows 10