1621162 - storage.onChanged fires for items that does not changed

Status: RESOLVED WONTFIX

(WebExtensions :: Storage, defect)

Description

[kernp25]

Attachment: manifest.zip

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

Steps to reproduce:

Install test add-on.

Actual results:

The changes object passed to storage.onChanged listener also contains items that does not changed.

Expected results:

When no items changed, storage.onChanged should not fire (like in Chrome).
If only 1 item changed, the changes object passed to storage.onChanged listener should also contain only the 1 changed item (not all).

Comment 1

[kernp25]

If this is not a bug, should this be documented on mdn?

Comment 2

[kernp25]

Attachment: chrome_SnFJ9NqlKJ.png

How it looks like in Google Chrome.

Comment 3

[kernp25]

Attachment: firefox_OScR8RsYAD.png

In Firefox it fires also, if no items changed.

Comment 4

[kernp25]

Attachment: firefox_YSQRXAPM9a.png

In Firefox all items will be passed to the listener, even if they not changed.

Comment 5

[Alex Cornestean]

Hello,

I’ve managed to reproduce the issue on the latest Nightly (76.0a1/20200316154314), Beta (75.0b4/20200313173516), Release (74.0/20200309095159) and ESR (68.6.0esr/20200305175243) under Windows 10 Pro 64-bit and Ubuntu 16.04 LTS.

All items are passed to the listener even if they do not change values.

The steps to reproduce I’ve employed are the following:

1. Load attached add-on in about:debugging
2. Open options page of the add-on
3. Click the save button
4. Click the save button several more times
5. Check a checkbox and click the save button
6. Click the save button again
7. Click the save button several more times

Note: Steps 4 and 7 are necessary to show all listeners firing even though there is no change in values. Results can be checked in the add-on console.

I’ve also checked whether the issue might be a regression (running a regression range from 2018-01-01 up to the current date), however, it seems that this issue in not one.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 7

[kernp25]

(In reply to kernp25 from comment #1)

If this is not a bug, should this be documented on mdn?

Comment 8

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

(In reply to kernp25 from comment #7)

(In reply to kernp25 from comment #1)

If this is not a bug, should this be documented on mdn?

yes, this is currently the expected behavior (and it didn't changed, the same was also true for the previous JSON backend, and so I'm removing the regression keyword).

Only emitting the onChanged events when the value is actually different is going to be potentially expensive, it looks pretty simple for some primitive type (like a boolean), but the value could be a deeply nested object and that would be much more expensive.

In my opinion it would be even more unexpected and confusing if storage.onChanged would behave differently on primitive and deeply nested object values, and so I think that it would be more reasonable to clarify the expected behavior on the API docs side, and so I'm marking this as dev-doc-needed.

Comment 10

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

would this bug marked as dev-doc-needed still be visible from an MDN perspective if I close it as wontfix?
(the only actionable item here is to reflect the expected behavior in the API docs).

Comment 11

[Richard Bloor]

It would still be visible as a dev-doc-needed if this was closed as wontfix, however I've setup a ticket on GitHub. What priority would you give this?

Comment 12

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

(In reply to Richard Bloor from comment #11)

It would still be visible as a dev-doc-needed if this was closed as wontfix, however I've setup a ticket on GitHub.

Thanks for the confirmation Richard, would you mind to add the github ticket as a see also on this issue once you have filed it?
In the meantime I'll close this as wontfix (and just pending the changes on the API docs side).
(I'm adding a needinfo assigned to you, so that I'll be notified of the github ticket linked as a see also once you clear the needinfo, even if the issue is going to be closed).

What priority would you give this?

In my own opinion we can make it a P2 (or a P3), because it isn't super-critical (as I mentioned this has been the behavior of the storage.onChanged event right from the start, and for most of the extensions it isn't making any actual difference, but it would be better to explicit what is the expected behavior in the API docs).

Comment 13

[Richard Bloor]

Sorry for the delay, here is the URL for the ticket: https://github.com/mdn/sprints/issues/2977

Comment 14

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

Thanks Richard, I just added it as a see also on this bugzilla issue.

Comment 15

[Richard Bloor]

Documentation updated in storage.onChanged clarrification #18326