1478009 - Ublock Origin cannot reset its own settings

Status: RESOLVED INVALID

(WebExtensions :: Storage, defect)

Description

[gwarser]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180724100052

Steps to reproduce:

- Firefox Beta (62) or Nightly (63) fresh profile 
- go to about:addons -> extensions
- search for Ublock Origin (1.16.14) and install from AMO
- go to uBlock Origin (uBO) Preferences (from addons manager)
- go to Filter list tab
- click on "+ Regions, languages" and select first three lists
- click "Apply changes" in top right corner
- wait for "XXX out of YYY" messages appear near the selected lists
- ctrl+shift+j for console 
- switch to uBO "Settings" tab
- click on "Reset to default settings..." button and confirm in dialog (OK)
- uBO will restart 






Actual results:

- browser console shows:

    Firefox Beta:
    An IndexedDB transaction that was not yet complete has been aborted due to page navigation. vapi-cachestorage.js:278:22
    [Show/hide message details.] TypeError: extension is undefined
    receiveMessage@resource://gre/modules/ExtensionParent.jsm:349:1
    _handleMessage/</<@resource://gre/modules/MessageChannel.jsm:912:19
    _handleMessage/<@resource://gre/modules/MessageChannel.jsm:911:9
    _handleMessage@resource://gre/modules/MessageChannel.jsm:909:7
    receiveMessage/<@resource://gre/modules/MessageChannel.jsm:208:9
    receiveMessage@resource://gre/modules/MessageChannel.jsm:201:5
    MessageChannel.jsm:914
    _handleMessage/</<
    resource://gre/modules/MessageChannel.jsm:914:11


    Firefox Nightly:
    An IndexedDB transaction that was not yet complete has been aborted due to page navigation. vapi-cachestorage.js:278:22
    [Show/hide message details.] TypeError: extension is undefined
    receiveMessage@resource://gre/modules/ExtensionParent.jsm:351:5
    async*_handleMessage/</<@resource://gre/modules/MessageChannel.jsm:922:19
    _handleMessage/<@resource://gre/modules/MessageChannel.jsm:921:9
    _handleMessage@resource://gre/modules/MessageChannel.jsm:919:7
    receiveMessage/<@resource://gre/modules/MessageChannel.jsm:218:9
    receiveMessage@resource://gre/modules/MessageChannel.jsm:211:5
    MessageListener.receiveMessage*FilteringMessageManager@resource://gre/modules/MessageChannel.jsm:201:5
    get@resource://gre/modules/MessageChannel.jsm:436:14
    addListener@resource://gre/modules/MessageChannel.jsm:763:7
    init@resource://gre/modules/ExtensionParent.jsm:276:5
    init@resource://gre/modules/ExtensionParent.jsm:503:7
    startup@resource://gre/modules/Extension.jsm:1766:7
    async*startup/this.startupPromise<@resource://gre/modules/LegacyExtensionsUtils.jsm:195:7
    startup@resource://gre/modules/LegacyExtensionsUtils.jsm:142:27
    start@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/on/Apps/firefox-nightly/browser/features/[e-mail]!/bootstrap.js:178:10
    handleStartup@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/on/Apps/firefox-nightly/browser/features/[e-mail]!/bootstrap.js:171:5
    promise callback*startup@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///home/on/Apps/firefox-nightly/browser/features/[e-mail]!/bootstrap.js:134:23
    callBootstrapMethod@resource://gre/modules/addons/XPIProvider.jsm:1588:20
    async*startup@resource://gre/modules/addons/XPIProvider.jsm:1703:27
    async*startup@resource://gre/modules/addons/XPIProvider.jsm:2150:13
    callProvider@resource://gre/modules/AddonManager.jsm:206:12
    _startProvider@resource://gre/modules/AddonManager.jsm:654:5
    startup@resource://gre/modules/AddonManager.jsm:813:9
    startup@resource://gre/modules/AddonManager.jsm:2811:5
    observe@jar:file:///home/on/Apps/firefox-nightly/omni.ja!/components/addonManager.js:66:9
    MessageChannel.jsm:924
    _handleMessage/</<
    resource://gre/modules/MessageChannel.jsm:924:11
    [Show/hide message details.] No matching message handler for the given recipient. MessageChannel.jsm:924
    _handleMessage/</<
    resource://gre/modules/MessageChannel.jsm:924:11


- go to uBO Preferences ->  Filter lists tab
- previously selected filter lists are still selected


Expected results:

Filter lists selection should reset to default - this happens in Firefox stable and Chromium

Comment 1

[gwarser]

Bug report in uBlock Origin repository https://github.com/uBlockOrigin/uBlock-issues/issues/144
Workaround https://github.com/gorhill/uBlock/commit/7ae68c8d7db188bc6f20e61fc0172297ceb32eeb

Comment 2

[Kris Maglione [:kmag]]

This seems like a bug in the extensions.

Comment 3

[Gingerbread Man]

"I implemented a workaround. The issue is really in Firefox, as the same code worked fine in Chromium. But since this is a key feature, workaround is needed. I believe the change in behavior in Firefox might be caused by moving browser.storage.local from JSON-based file to indexedDB-based storage. I still see related message in the browser console, but I could reset/restore fine at least."
https://github.com/uBlockOrigin/uBlock-issues/issues/144#issuecomment-407403406

Comment 4

[Kris Maglione [:kmag]]

Just because something works in Chromium, that doesn't mean it's correct. Firefox aborts any indexedDB transactions that haven't completed when a page unloads. I believe this is the correct behavior. If the same code works in Chromium, it either means that it doesn't abort those transactions (which sounds like a bug), or there's a race, and the transactions happen to finish there before the extension restart operation tears down the options page.

Either way, that means the extension is relying on undefined behavior.

Comment 5

[gwarser]

Gorhill replied:

>Disabling an extension (which is the first step of a restart operation) is not the equivalent of crashing the extension. In a correct implementation, stopping an extension involves flushing out all pending operations, including storage write operations. That's why it works fine with Chromium.

>Also, keep in mind that the callback parameter in `browser.storage.local` set/clear methods are optional, but with the new Firefox implementation of the API it can be said the parameter is no longer optional, it's key for the write operations to actually succeed.

>Consider that a OS does not discard buffered written data to a file when the process owning the file handle terminates, the OS flushes out the buffered data -- it's not unreasonable to expect the same from `browser.storage.local`. Also think of `navigator.sendBeacon` which will still cause a network request to be fired even after the page which made the call is unloaded.

and

>>the extension is relying on undefined behavior

>So I suppose [the documentation](https://developer.mozilla.org/en-US/docs/Web/API/IDBTransaction) needs updating, "unloading the document which fired the transaction" is not listed in the reasons for aborting a transaction.

https://github.com/uBlockOrigin/uBlock-issues/issues/144#issuecomment-407718261

Comment 6

[Kris Maglione [:kmag]]

This isn't the equivalent of the OS failing to flush buffered data. It's the equivalent of a database client disconnecting before it finalizes its current transaction. That transaction is incomplete, and therefore the obvious course of action is to roll it back rather than commit it. There's no way for the DB engine to know that there aren't further parts of the transaction that are required for consistency, and therefore committing it could lead to data corruption.

And it also has nothing to do with storage.local, as far as I can tell. The file in question is using indexedDB directly.

Comment 7

[[e-mail]]

My bad then, sorry for the noise.

I had a strongly anchored understanding that write operations of `browser.storage.local` were queued tasks, i.e. as soon as the call returned, the task was going to be undertaken at some point (successfully or not) regardless of what happens to the caller (just like the `sendBeacon` example).

That understanding was based on what I had observed with Chromium over the years, and the documentation was not especially contradicting this. Turns out the real way to understand it is that it's a database transaction, at risk of being aborted after the call returns, like if the extension is stopped in any ways.

Comment 8

[Kris Maglione [:kmag]]

(In reply to [e-mail] from comment #7)
> I had a strongly anchored understanding that write operations of
> `browser.storage.local` were queued tasks, i.e. as soon as the call
> returned, the task was going to be undertaken at some point (successfully or
> not) regardless of what happens to the caller (just like the `sendBeacon`
> example).

This is true for browser.storage.local transactions. indexedDB operations are database transactions, though, and the script in question is using indexedDB directly.

Comment 9

[[e-mail]]

> This is true for browser.storage.local transactions

I am back to being confused then. The extension which exhibits the issue described is using browser.storage.local (which is directly aliased as vAPI.storage) for its settings -- the indexedDB part aka vAPI.cacheStorage is used solely for discardable data and is not related to the issue described here.

The settings not being cleared after calling browser.storage.clear[1] and forcing a restart without waiting for the result[2] are the ones saved in browser.storage.local.


[1] https://github.com/gorhill/uBlock/blob/99521b3b2bb84f6d8d4400793322bba51f3efd30/src/js/messaging.js#L801
[2] https://github.com/gorhill/uBlock/blob/99521b3b2bb84f6d8d4400793322bba51f3efd30/src/js/messaging.js#L807

Comment 10

[gwarser]

I can't reproduce anymore after this landed: https://bugzilla.mozilla.org/show_bug.cgi?id=1477015

>INFO: No more inbound revisions, bisection finished.
>INFO: First good revision: d7aac87e2db54dc57b951e63079f053df49a50ef
>INFO: Last bad revision: f32e9ff50ebd4e63205491905087ab7dee11b506
>INFO: Pushlog:
>https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f32e9ff50ebd4e63205491905087ab7dee11b506&tochange=d7aac87e2db54dc57b951e63079f053df49a50ef

Comment 11

[gwarser]

To be clear - I still see errors in console, but settings are properly cleared.