1561584 - storage.StorageChange object has no oldValue when transitioning from a falsey value

Status: RESOLVED FIXED

(WebExtensions :: Storage, defect, P3)

Description

[Clint Priest]

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

Steps to reproduce:

This rough pseudo-code demonstrates the issue

browser.storage.sync.set('test', false); // await yields no rejection
browser.storage.onChanged.addListener(...onChanged) // register onChanged listener
browser.storage.sync.set('test', true);

Actual results:

// onChanged listener is called with missing oldValue key/value in StorageChange object

Expected results:

// onChanged listener is called with oldValue set to false in StorageChange object

Comment 1

[Hani Yacoub, Desktop QA]

Is there a way to reproduce this issue with more clearer steps?
Thanks.

Comment 2

[Clint Priest]

Apologies, I thought the pseudo-code above would be enough, I'll try and post a sample a bit later tonight.

Comment 3

[Clint Priest]

Here is some sample code, this would need to run in the context of a web extension which has access to browser.storage:

window.addEventListener('DOMContentLoaded', async () => {
	// register onChanged listener
	console.log('registering change listener');
	browser.storage.onChanged.addListener((aChanges) => {
		console.log(aChanges.test);
	});

	await browser.storage.sync.set({ 'test': false });
	await browser.storage.sync.set({ 'test': true });
	await browser.storage.sync.set({ 'test': false });
	await browser.storage.sync.set({ 'test': true });
	await browser.storage.sync.set({ 'test': false });

	console.log('done setting test to true');
}, { once: true, passive: true });

Console Log Shows

     registering change listener
    	Object { newValue: false }
    	Object { newValue: true }
    	Object { newValue: false, oldValue: true }
    	Object { newValue: true }
    	Object { newValue: false, oldValue: true }
     done setting test to true

Console Log Should Show

     registering change listener
     	Object { newValue: false }
     	Object { newValue: true, oldValue: false }
     	Object { newValue: false, oldValue: true }
     	Object { newValue: true, oldValue: false }
     	Object { newValue: false, oldValue: true }
     done setting test to true

Comment 4

[Hani Yacoub, Desktop QA]

Assigning "Core: DOM: Web Storage" component for this and hopefully someone with more knowledge in this area will a look over this.

Comment 5

[Myeongjun Go]

Attachment: Bug 1561584 - storage.StorageChange object has no oldValue when transitioning from a falsey value

Comment 6

[Myeongjun Go]

Hello.
I submitted patch.
When 'old value' is boolean type, It is not working right. I modified to pass the conditional statement in this case.
Tell me if I need anything or if there's a better case.

Comment 7

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

(In reply to Myeongjun Go from comment #6)

Hello.
I submitted patch.
When 'old value' is boolean type, It is not working right. I modified to pass the conditional statement in this case.
Tell me if I need anything or if there's a better case.

Looking to that code it seems that the "boolean false" is not the only falsey value that is triggering this bug, have you looked into the behavior with other falsey value that may be triggering the same issue?
(e.g. see https://developer.mozilla.org/en-US/docs/Glossary/Falsy for the list of other values that are considered falsey).

We should also add an explicit test case for this bug, as it is clearly missing right now :-)

Feel free to add me as a reviewer, that way I'll be notified automatically when you update the patch.

Comment 8

[kernp25]

Can you just do?

typeof oldRecord.data !== "undefined"

Comment 9

[kernp25]

Or just?

      if (oldRecord) {
        // Extract the "data" field from the old record, which
        // represents the value part of the key-value store
        changes[key].oldValue = oldRecord.data;
      }

Will this also work?

Comment 10

[kernp25]

But i think:

"data" in oldRecord

should also work.

Comment 11

[Clint Priest]

Sure, I can (and have) worked around the bug, doesn't make it any less of a bug.

The problem is not that it is a falsey value, the problem is that it is omitting the value.

I explicitly set it to false and it is coming back as missing, not even set to undefined or false.

Comment 12

[Myeongjun Go]

rpl, Thanks for reviewing.
I solved issue in the case just 'false'.
But It happens same issue with other falsey. I didn't solved in case other 'falsy'(undefined, null ... ).
I gonna write code again to solve other 'falsy' and test case :)

Comment 13

[Myeongjun Go]

(In reply to kernp25 from comment #8)

Can you just do?

typeof oldRecord.data !== "undefined"

Thanks for reviewing :)
This code give me reference can't solve this issue. Because The issue have not only 'undefined' case.

Comment 14

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

(In reply to kernp25 from comment #9)

Or just?

      if (oldRecord) {
        // Extract the "data" field from the old record, which
        // represents the value part of the key-value store
        changes[key].oldValue = oldRecord.data;
      }

Will this also work?

yes, that is what I also suggested in the review comments on phabricator.

Comment 15

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

There is a patch in review for this, and so I'm assigning the issue to Myeongjun Go.

Comment 16

[Caitlin Neiman [:caitmuenster]]

Hey Myeongjun, how's it going with this bug?

Comment 17

[Myeongjun Go]

I'm sorry for being late.
I recently started working on the code and will submit the patch soon.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d31c956ee18
storage.StorageChange object has no oldValue when transitioning from a falsey value r=rpl

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6d31c956ee18

Comment 20

[Clint Priest]

Thanks, to everyone for getting this fixed!