1802929 - Refresh items button does not refresh extension storage

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect, P3)

Description

[Juha-Matti Santala [:juhis]]

Thank you for helping make Firefox better. If you are reporting a defect, please complete the following:

What were you doing?

I have an extension that modifies data in the storage.local. When inspecting that via DevTools in Storage tab, I'd expect to see the values being updated when clicking Refresh items.

I have set up a minimal extension that can be used to test it: https://github.com/Hamatti/bugreport-devtools

1. Load the extension to a Firefox instance (I tested with 107.0).
2. Go to about:debugging and select Inspect for the new extension, named "Example for storage.local"
3. Go to Storage tab and select this extension's storage.
4. Click on the extension button a few times in the browser.
5. Click the Refresh icon

What happened?

Nothing happens. No errors but the view does not update.

What should have happened?

I expect the view to update and reflect the current state of data stored in storage.local

Anything else we should know?

I tested this in 107, 108 beta and 109 nightly by loading the extension with web-ext run

Comment 1

[Julian Descottes [:jdescottes]]

Hi Luca,

On DevTools side, our Storage actor relies on the listeners provided at https://searchfox.org/mozilla-central/rev/a3c18883ef9875ba4bb0cc2e7d6ba5a198aaf9bd/toolkit/components/extensions/ExtensionStorageIDB.jsm#846-869 to update itself.

But in the scenario provided by :juhis here, notifyListeners is never called. I can see that it's supposed to be called from parent ext-storage.js , but I can't debug this file using the Browser Toolbox, maybe it's not loaded at all. Should we listen to something else or is there something wrong with this API?

Comment 2

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

(In reply to Julian Descottes [:jdescottes] from comment #1)

Hi Luca,

On DevTools side, our Storage actor relies on the listeners provided at https://searchfox.org/mozilla-central/rev/a3c18883ef9875ba4bb0cc2e7d6ba5a198aaf9bd/toolkit/components/extensions/ExtensionStorageIDB.jsm#846-869 to update itself.

But in the scenario provided by :juhis here, notifyListeners is never called. I can see that it's supposed to be called from parent ext-storage.js , but I can't debug this file using the Browser Toolbox, maybe it's not loaded at all. Should we listen to something else or is there something wrong with this API?

Hi Julian,
sorry for not getting to look into this issue sooner.

I just took a quick look into it and I see what is going on:

• When parent/ext-storage.js is loaded, it does subscribe to the "Extension:StorageLocalOnChanged:EXTUUID" events on the parent process message manager here: https://searchfox.org/mozilla-central/rev/a0d4f8f112c5c792ae272bf6ce50763ddd23ffa2/toolkit/components/extensions/parent/ext-storage.js#65-71

• That parent process message manager listener is what is going to notify the storage.onChanged listeners from here (and the listeners registered internally by the storage API implementation itself are what forward the notification coming from one child process also to all other child processes where an extension context exists, e.g. a content script that may have registered a storage onChange listener): https://searchfox.org/mozilla-central/rev/a0d4f8f112c5c792ae272bf6ce50763ddd23ffa2/toolkit/components/extensions/parent/ext-storage.js#180-184

• ... but it is also what should makes sure that the listener that the devtools storage actor is being notified of extension storage changes and the storage panel to be updated accordingly.

It is pretty common for an extension that uses the storage API to also subscribe a storage.onChanged event, and if there is none then the fact that we don't forward the API event to other contexts running in other processes is expected, but that didn't consider the listener that the devtools Storage panel is registering.

As an additional confirmation that what I've described above is the actual issue:

• the minimal test extension attached by Juhis does call browser.storage.local.get and browser.storage.local.set (which are both implemented in child/ext-storage.js and so they are storage APIs that would not make Firefox to load parent/ext-storage.js as a side-effect) and so it is definitely expected for the minimal test extension in this form to be triggering the issue reported here.

• But, if we add a storage API call that is implemented in the parent process (which to be handled is expected to load parent/ext-storage.js in the parent process as a side effect), then the panel gets refreshed as expected (and setting breakpoints or watchpoints around the devtools/actor/storage.js code that is expected to be notified about the storage data change gets called and the call is originated from https://searchfox.org/mozilla-central/rev/a0d4f8f112c5c792ae272bf6ce50763ddd23ffa2/toolkit/components/extensions/parent/ext-storage.js#180-184 as expected), e.g.:

  • adding browser.storage.local.onChanged.addListener(() => {}) does prevent the issue to be triggered and the storage panel gets updated as expected
  • but also just calling await browser.storage.managed.get({}) once would have the same side-effect (and does also demonstrate that it is triggering parent/ext-storage.js to be loaded is what prevents the issue from being triggered, and there isn't strictly the fact that the extension does subscribe its own storage onChanged listener)

From a bug tracking perspective:

• This seems something that we should be fixing on the WebExtensions ext-storage.js API scripts and not in the devtools storage actor, and so technically this issue may belong more to "WebExtensions :: Storage" bugzilla component than in "DevTools :: Storage Inspector", even if the issue wouldn't be actually reproducible nor noticeable without the devtools storage inspector
• Nonetheless, as a user (extension developer) experiencing this issue I would also look for a pre-existing bug report into the "DevTools" components, given that it is the only place where as an extension developer I could both trigger and notice the issue, and so it may be still be worth tracking it in the current bugzilla component.

Anyway, In the meantime, I'm still assigning this to me to look into a proper fix for the underlying issue.

Comment 3

[Julian Descottes [:jdescottes]]

Thanks for the detailed info Luca. It's fine to keep this in this component of course, as you said, the issue is visible only in DevTools. I'll let you investigate the best way to fix this, but if there's anything we can do in DevTools to make this easier (eg force to load parent ext-storage?), let me know!

Comment 4

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

Attachment: Bug 1802929 - Fix missing DevTools storage panel updates on extension with no browser.storage onChanged listeners. r?jdescottes!,willdurand!

Comment 5

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

Push to try: https://treeherder.mozilla.org/jobs?repo=try&revision=a95bb4a98cf76500bcfe10fb7d95f4ef961e2818

Comment 6

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

Attachment: Bug 1802929 - Prevent XULStore "Can't find profile directory" exception to trigger test_extension_storage_actor.js failures. r?ochameau!

While investigating a TV job failure from my push to try for the other patch attached to this issue,
I did notice that the "Error: Can't find profile directory" raised from XULStore load method that
was triggering that failure was a side-effect of the fact that the fallback document
chrome://devtools/content/shared/webextension-fallback.html nodePrincipal is a system principal
and that is making Document:SetReadyStateInternal to call XULPersist::Init (which they tries to
initialize XULStore in the child extension process, where it is not going to work).

Fixing that properly is already tracked by Bug 1698087, and it is likely to require more changes
on the devtools side (e.g. the one tracked by Bug 1754452), and so as a smaller short term test-only
workaround I'm proposing to introduce a new hidden pref which we could use in the few tests that
would be hitting it to fallback to about:blank instead (which doesn't seem to be triggering the
same side-effect, very likely just because it is not a chrome url).

Depends on D166361

Comment 7

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

New push to try (which confirms that the new patch attached in comment 6 prevents the TV job failure that was caught by the previous push to try linked in comment 5):

• https://treeherder.mozilla.org/jobs?repo=try&revision=a3cfa379229cee7008868a11c33dd938c98efcf4

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/35f30b8bc3ed
Fix missing DevTools storage panel updates on extension with no browser.storage onChanged listeners. r=jdescottes,willdurand

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/35f30b8bc3ed