1554088 - Promise returned by `browser.storage.local.set` is fulfilled before `storage.onChanged` listener is executed

Status: RESOLVED INVALID

(WebExtensions :: Storage, defect)

Description

[juraj.masiar]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

1. open any add-on page
2. open Web-Console
3. execute following code:

(async () => {
  let text = '...';
  browser.storage.onChanged.addListener(onStorageChange);

  console.log('EXPERIMENT: before', text);
  // we await setting new value - this will trigger "onChanged" handler and mutate "text"
  await browser.storage.local.set({experiment: Date.now()});
  // text value should be changed now
  console.log('EXPERIMENT: after', text);

  browser.storage.onChanged.removeListener(onStorageChange);
  
  function onStorageChange(changes) {
    text = 'text mutated!!!';
    console.log('EXPERIMENT: onChanged', text);
  }
})();

Actual results:

Console will log:

EXPERIMENT: before ...
EXPERIMENT: after ...

Expected results:

EXPERIMENT: before ...
EXPERIMENT: onChanged text mutated!!!
EXPERIMENT: after text mutated!!!

This works in ESR 60 and Chrome. It doesn't work in Firefox 66 - 69, I didn't tested 61-65.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

That is actually the intended behavior, in the storage.local backend enabled in 66 the promise returned by storage.local.set is resolved once the underlying storage transaction has been completed (previously storage.local.set used to resolve before the data was actually stored, and the data could even fail silently to be stored without the extension to ever know), and the onChanged event is fired once that happens.

I'm closing this issue as invalid (because the actual behavior is the expected one, and the issue description doesn't provide any reasons to think that the current behavior is not the right).

Comment 2

[juraj.masiar]

It feels wrong. Are you sure about it? Isn't there some spec that defines the behavior?

This complicates cases where you need your in-memory data in sync with stored data - because after you change it in storage, there is no way to await the propagation through the onChanged handler and the in-memory version stays unchanged for some time.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to juraj.masiar from comment #2)

It feels wrong. Are you sure about it? Isn't there some spec that defines the behavior?

there are no official specs for the storage WebExtension API ([1])

This complicates cases where you need your in-memory data in sync with stored data - because after you change it in storage, there is no way to await the propagation through the onChanged handler and the in-memory version stays unchanged for some time.

can you provide a reduced test case that shows an actual scenario where this becomes a problem that can't be solved otherwise?

Let's say, as an hypothesis, that the storage.set would be resolved after the storage.onChanged has been fired, the data is still already stored even before the onChanged event is fired and then received by the subscribed listeners, if in the meantime the same extension calls storage.local.get the result got would still be out of sync with the in-memory version (and firing storage.onChanged before the data is actually stored sounds like the wrong behavior to me).

[1]: the only formal spec for the WebExtensions API is the draft spec from https://browserext.github.io/browserext/, it has never been updated from the 2017, and it doesn't seem to even ever included the storage API).

Comment 4

[juraj.masiar]

Well, to make things easy and to save performance, I've created a module that synchronizes stored data with in-memory constant:

import {replaceItemsInArray} from "./array_utils.js";

export async function registerStorageLoader(name, targetArray) {
  // register storage change listener
  browser.storage.onChanged.addListener(onStorageChange);
  // load current value
  const {[name]: storedData = []} = await browser.storage.local.get([name]);
  return replaceItemsInArray(targetArray, storedData);

  function onStorageChange(changes) {
    if (changes[name]) {
      replaceItemsInArray(targetArray, changes[name].newValue || []);
    }
  }
}

Another module then exposes the user data as simple constant:

// user data are stored in memory in "BIG_USER_DATA" array which is mutated when data are changed
export const BIG_USER_DATA = [];    // should be below 1MB
// the promise is awaited at the app start to make sure data are ready
export const BIG_USER_DATA_PROMISE = registerStorageLoader('user_data', {targetArray: BIG_USER_DATA});

Now the problem is, that updating the data using await browser.storage.local.set will make my in-memory data out of sync and any code executed right after the update will have old data.
I know the problem can be fixed by multiple solutions, but all of them will require a lot of refactoring as my codebase is huge.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

For an API event named storage.onChanged the main guarantee should be that it is going to be fired after the data is being stored, firing it before the data is being fully stored would make it more of a storage.onChanging than storage.onChanged.

I know the problem can be fixed by multiple solutions, but all of them will require a lot of refactoring as my codebase is huge.

Well, I'm sorry about that, but it still doesn't sound like an issue of the API event, but more of an assumption made by the extension on an undocumented (and so also unsupported) API event behavior.

I guess that one way to fix it on the extension side (without doing a big refactoring) may be to make the extension itself to emit its own "onChanging" event right before calling the storage.local.set event (well, and also take into account what should happen if the storage.local.set call returns a rejected promise).

Comment 6

[juraj.masiar]

okok, thank you for the info!

I've fixed my issue by manually updating in-memory version in those critical parts where updated data are required right away.

Comment 7

[juraj.masiar]

I have one more question regarding current underlying implementation.
If I register multiple browser.storage.onChanged listeners, when they are executed, they are executed synchronously one by one, right?

If I call setTimeout in one of them with some callback fn, I can be 100% sure the callback will be executed after all existing onChanged listeners fired, right?

For example:

browser.storage.onChanged.addListener(changes => { console.log(1); });
browser.storage.onChanged.addListener(changes => { console.log(2); });
browser.storage.onChanged.addListener(changes => { console.log(3); });
browser.storage.onChanged.addListener(changes => {
  setTimeout(() => {
    console.log(`I can be 100% sure now that all other handlers fired, right?`);
  }, 0)
});

Thank you!