1541449 - browser.storage.onChanged doesn't fire when false values are removed

Status: RESOLVED FIXED

(WebExtensions :: Storage, defect, P1)

Description

[Dave Vandyke]

Attachment: storage-onchanged-test.zip

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/72.0.3626.119 Safari/537.36

Steps to reproduce:

1. Install attached extension, opened the background console for it.

Actual results:

The following was logged:

onChanged example: { oldValue: undefined, newValue: false } local

Expected results:

The following should be logged:

onChanged example: { oldValue: undefined, newValue: false } local
onChanged example: { oldValue: false } local

Comment 1

[Dave Vandyke]

The event works as expected on both older versions of Firefox and Chrome:

Firefox 64.0 (64-bit)

onChanged example: { oldValue: undefined, newValue: false } local
onChanged example: { oldValue: false } local

Chrome 72.0.3626.119

onChanged example: { newValue: false } local
onChanged example: { oldValue: false } local

See also the related discussion on the Adblock Plus issue tracker: https://issues.adblockplus.org/ticket/7430

Comment 2

[Dave Vandyke]

(Should I mark issues like this as blocking #467520 or #1226547?)

Comment 3

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

Attachment: Bug 1541449 - storage.local API should fire onChanged event when falsey values are removed. r?mixedpuppy!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/176a8c9d1b7b
storage.local API should fire onChanged event when falsey values are removed. r=mixedpuppy

Comment 5

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/176a8c9d1b7b

Comment 6

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

Let's QA verify the fix as applied in Nightly using the STR from comment 0.

Comment 7

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

Comment on attachment 9055516 [details]
Bug 1541449 - storage.local API should fire onChanged event when falsey values are removed. r?mixedpuppy!

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1488825
• User impact if declined: Some extension may not behave as expected because the expected onChanged event is not being fired when the changed item has a falsey value (e.g. as mentioned in comment 1, AdBlock Plus doesn't update part of its UI because the expected onChanged event has not been fired).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: QA verification of the fix can be done using the STR provided by the reporter in comment 0.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change required to fix the issue is small and restricted to the internal function that implements the underlying behavior for the storage.local.remove API method.
  The change also includes additional test cases to ensure that this scenario is explicitly tested as part of the automated tests.
• String changes made/needed:

Comment 8

[Pascal Chevrel:pascalc (PTO until April 26)]

I'll take it in Beta once QA has verified it in Nightly, thanks for the STR!

Comment 9

[Anca Soncutean, Desktop QA]

I’ve reproduce this issue following the steps from comment 0 with Fx 68.0a1(2019-04-03) on Ubuntu 18.04 x64.
The issue is verified fixed with Fx 68.0a1 (2019-04-10) on Ubuntu 18.04 x64, macOS 10.13 and Windows 10 x64.

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9055516 [details]
Bug 1541449 - storage.local API should fire onChanged event when falsey values are removed. r?mixedpuppy!

Impacts some high volume extensions, has tests and was veirfied by QA po Nightly, uplift approved for 67 beta 11, thanks!

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8f56f6681da4

Comment 12

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

Comment on attachment 9055516 [details]
Bug 1541449 - storage.local API should fire onChanged event when falsey values are removed. r?mixedpuppy!

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1488825
• User impact if declined: Same as comment 7
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: QA verification of the fix can be done using the STR provided by the reporter in comment 0.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Same as comment 7
• String changes made/needed:

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

So far, we don't have another dot release planned, but I'll keep this open for another week in case something else crops up.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9055516 [details]
Bug 1541449 - storage.local API should fire onChanged event when falsey values are removed. r?mixedpuppy!

We're a week or so out from 67 release. Wontfix for 66.