Create test WE for Bug 1292234 #3

biancadanforth · 2019-03-01T21:06:16Z

See ./bug-1292234-test-we/README.md for context

rhelmer · 2019-03-01T21:32:21Z

One meta-comment I should have done earlier - I don't think the mdn/webextensions-examples repo is the right place for this, since it'll cause confusion for regular third-party extension devs.

I think https://docs.telemetry.mozilla.org/ is probably the right place since it's pretty specific to Mozilla's experiments program (for instance the client guidelines https://docs.telemetry.mozilla.org/cookbooks/client_guidelines.html).

Maybe http://firefox-source-docs.mozilla.org/ but I feel like that's kind of stretch, I think the above is probably a better place for the documentation side of this.

The actual demo extension should live somewhere too but that could just be a GH repo in the mozilla/ namespace IMHO shouldn't be the generic MDN webextension one.

rhelmer

Just nits take or leave it.

rhelmer · 2019-03-01T21:33:54Z

bug-1292234-test-we/background.js

+    console.log("WebExtensionLog::ExtensionLocalStorageChange", changes);
+  }
+
+  // Add listeners


I don't think this comment adds clarity, since it's just describing what is happening on the next line.

If you want to document why adding a handler is a good idea here, that would be worth commenting.

Good point thanks

rhelmer · 2019-03-01T21:34:31Z

bug-1292234-test-we/web-ext-config.js

+module.exports = {
+  run: {
+    // Points to my local artifact development build of Firefox
+    firefox: '/Users/bdanforth/src/mozilla-unified/objdir-frontend-debug-artifact/dist/Nightly.app/Contents/MacOS/firefox',


This seem pretty specific for your setup :)

Is that bad? How might I abstract that away? One way or the other, I need to point to my local build.

rhelmer · 2019-03-01T21:34:53Z

bug-1292234-test-we/web-ext-config.js

+    // Points to my local artifact development build of Firefox
+    firefox: '/Users/bdanforth/src/mozilla-unified/objdir-frontend-debug-artifact/dist/Nightly.app/Contents/MacOS/firefox',
+    startUrl: [
+      'about:debugging',


I didn't know web-ext supported this that's super cool.

biancadanforth · 2019-03-05T18:16:21Z

@rhelmer Thanks for your feedback.

One meta-comment I should have done earlier - I don't think the mdn/webextensions-examples repo is the right place for this, since it'll cause confusion for regular third-party extension devs.

This is my own personal fork of the repo, I am not opening PRs onto the upstream MDN repo, nor am I using this repo for any other reason than my own personal experimentation and WE prototyping. Isn't that one of the use cases of forks -- to extend some code for your own purposes?

I agree that I have only added a few of my own example extensions here, and they could probably all live in a separate repo from this fork.

I think https://docs.telemetry.mozilla.org/ is probably the right place since it's pretty specific to Mozilla's experiments program (for instance the client guidelines https://docs.telemetry.mozilla.org/cookbooks/client_guidelines.html).

I'm a little confused what part of this would go into official Firefox docs?

These extensions are not intended to be examples for a larger audience; just as a place for me to try things out myself. I do think if/when a fix for Bug 1292234 lands, we could add a section to the doc you created about preferred use of preferences in experiments and link to an example extension hosted somewhere more official.

rhelmer · 2019-03-05T18:22:13Z

OK good point about just using this in your personal repo :) Since it was an MDN webextensions-examples fork I assumed you intended to ultimately land it in the upstream repo and was suggesting other places this could go potentially but you're right that this isn't really necessary. Sorry for the confusion.

In ./devtools/shared/specs/storage.js, the `storeObjectType` provides a schema of sorts our [WIP patch](https://gist.github.com/biancadanforth/fb6aaae07084512a594a8098c971807e), which currently uses an existing `storeObjectType` of `"storagestoreobject"`. This is the `storeObjectType` used by the window.localStorage and window.sessionStorage actors. However, this `"storeagestoreobject"` type [limits the data types](https://searchfox.org/mozilla-central/source/devtools/shared/specs/storage.js#119-122) of values stored to strings. Extension storage like `browser.storage.local` have fewer restrictions on the types of values that can be stored compared to window.localStorage and window.sessionStorage. Extension storage key-value type restrictions are most similar to (but more restrictive than[1]) `window.indexedDB`, so this patch puts some items in IndexedDB to compare how the IndexedDB storage actor handles and processes the data as a basis to create a new `storeObjectType` for extension storage. [1]: Compared to [`window.localStorage` and `window.sessionStorage`](https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage): > The values stored can be any JSON-ifiable value, not just String. Compared to [`window.indexedDB`](https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API#Key_concepts_and_usage): While extension storage can store any JSON-ifiable value, IndexedDB can store: > any objects supported by the [structured clone algorithm](https://developer.mozilla.org/en-US/docs/Web/Guide/API/DOM/The_structured_clone_algorithm) This includes Maps, for example and other complex objects.

This is to see what happens when there is no background page (i.e. by removing the 'background' key from 'manifest.json') for an extension (say it just has a popup) when I open the Storage inspector. Right now we are using the 'browser.storage' WE API directly off of the extension's 'window' object (e.g.: ``` // TODO: Don't use WE APIs directly, as a WE can overwrite the `browser` object async removeItem(host, name) { const win = this.storageActor.getWindowFromHost(host); await Cu.waiveXrays(win).browser.storage.local.remove(name); }, ``` ...but `window` not always available as in the case when there is no background page running in the extension and no other extension pages are open. I have confirmed this does indeed break with the following error: ``` Protocol error (unknownError): Error occurred while creating actor' server2.conn0.child13/storageActor8: TypeError: Cu.waiveXrays(...).browser is undefined ``` ...in ui.js:165:15. So we need to rely on something other than 'window' to get at the extension's storage. It may be possible if we can get the addonId some way to access the extension's storage via the privileged implementation of the storage WE API.

Because the storage actor (in mozilla-central/devtools/server/actors/storage.js) for Bug 1292234 is running in the extension child process, it cannot be debugged from the Browser Toolbox (debugger for main process), the addon debugger (debugger for web extension itself) or the in-content toolbox (debugger for web page scripts). With Fission, there will be a single toolbox that can target any different process, but in the meantime, there is a workaround to be able to debug this storage actor with the Browser Content Toolbox. 1) Open a webextension page of the target extension in a Firefox tab 2) Open the “Browser Content Toolbox” (from App Menu -> “Web Developers”) while the webextension page tab is currently selected (this way a toolbox window connected to the extension child process should be opened)

This only changes the part of the storage actor that is reading the stored data (`populateStoresForHost`), by: - checking in the extension's sharedData (extension data shared across the processes) if the IDB backend is enabled - It is possible this check returns `null`. In Firefox 66+ (i.e. Nightly), we use the IDB back-end by default, so a value of `null` means migration has not yet occurred. - With my [test WE](biancadanforth/webextensions-examples#3) which immediately puts a couple items into `browser.storage.local` from a background script, when running with `web-ext run` (and pointing to my local Nightly build with my patch), the extension must be reloaded once before opening the storage inspector to ensure the migration to the IndexedDB backend (which [happens lazily](https://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1940-1948)) has occurred. - As a fix to this, I have added a `null` check to trigger the migration in this scenario. - If the IDB backend is enabled (this check also means migration to the IDB backend has occurred), and we get the extension's storage principal from its sharedData object, and then use the [ExtensionStorageIDB.open method](https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/toolkit/components/extensions/ExtensionStorageIDB.jsm#648) to retrieve an instance of the [ExtensionStorageLocalIDB class](https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/toolkit/components/extensions/ExtensionStorageIDB.jsm#142) for that extension

This only changes the part of the storage actor that is reading the stored data initially each time the Storage Inspector is opened (when our storage actor's `populateStoresForHost` method is called), by: - checking in the extension's sharedData (extension data shared across the processes) if the IDB backend is enabled - If the IDB backend is enabled (this check also means migration to the IDB backend has occurred), we get the extension's storage principal from its sharedData object, and then use the [ExtensionStorageIDB.open method](https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/toolkit/components/extensions/ExtensionStorageIDB.jsm#648) to retrieve an instance of the [ExtensionStorageLocalIDB class](https://searchfox.org/mozilla-central/rev/8ff2cd0a27e3764d9540abdc5a66b2fb1e4e9644/toolkit/components/extensions/ExtensionStorageIDB.jsm#142) for that extension - This allows us to read the extension local storage data directly from our IDB backend and pass it along to the Storage Inspector. Notes: * With my [test WE](biancadanforth/webextensions-examples#3) which immediately puts a couple items into `browser.storage.local` from a background script, when running with `web-ext run` (and pointing to my local Nightly build with my patch), the extension must be reloaded once before opening the storage inspector to ensure the migration to the IndexedDB backend (which [happens lazily](https://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1940-1948)) has occurred. * The IDB backend is used by default in Nightly and Beta without needing to set the pref `extensions.webextensions.ExtensionStorageIDB.enabled` to `true`.

- It is possible this returns `null`. In Firefox 66+ (i.e. Nightly), we use the IDB back-end by default, so a value of `null` means IDB migration has not yet occurred as it [happens lazily](https://searchfox.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#1940-1948) the first time the `browser.storage` WE API is used, and it's possible that an extension calls this API before this code is listening for it. - This happens, for example, with my [test WE](biancadanforth/webextensions-examples#3) which immediately puts a couple items into `browser.storage.local` from a background script, when running it with `web-ext run` (and pointing to my local Nightly build with my patch). - A temporary workaround would be to reload the extension after Firefox has finished starting up and the code is ready and listening for the WE storage API calls. - This patch provides a permanent fix: it now correctly displays the test WE's storage items in the Storage Inspector without having to reload the extension by triggering a migration if one has not yet occurred. It does this without relying on message passing between the parent and child processes (i.e. does not need this.conn.setupInParent, the use of which we are trying to avoid in considering forward compatibility with upcoming Fission changes to DevTools, or similar)! Notes: * Just like all previous patches, none of these work for the case that the extension does not have a background page, as we are still relying on `window` objects (e.g. `Cu.waiveXrays(window).browser.storage...`) in places. Hopefully this will change with my next revision.

This helps debug the situation where an extension does not have a background script (and may therefore not have a window object available to our storage actor.

Create test WE for Bug 1292234

4f8fb95

rhelmer approved these changes Mar 1, 2019

View reviewed changes

biancadanforth force-pushed the bug-1292234 branch 2 times, most recently from c2fd293 to 6062d3b Compare March 6, 2019 23:15

biancadanforth force-pushed the bug-1292234 branch from 6062d3b to 7737265 Compare March 6, 2019 23:35

biancadanforth added 2 commits March 8, 2019 17:18

biancadanforth mentioned this pull request Mar 20, 2019

Fix #1: Replace Cu.waiveXrays with internal API calls for the IDB storage back end biancadanforth/bug_1542035_and_1573201#7

Merged

Open an extension page in popup

d8f075d

This helps debug the situation where an extension does not have a background script (and may therefore not have a window object available to our storage actor.

This was referenced Mar 26, 2019

Storage Inspector data is wrongly empty for extension without background page biancadanforth/bug_1542035_and_1573201#9

Closed

Storage Inspector data does not live update correctly when an extension is reloaded biancadanforth/bug_1542035_and_1573201#11

Closed

biancadanforth mentioned this pull request Apr 10, 2019

Storage item value wrongly changes to {} the second time a transient page's script runs biancadanforth/bug_1542035_and_1573201#16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Create test WE for Bug 1292234 #3

Create test WE for Bug 1292234 #3

Uh oh!

biancadanforth commented Mar 1, 2019 •

edited

Loading

Uh oh!

rhelmer commented Mar 1, 2019

Uh oh!

rhelmer left a comment

Uh oh!

rhelmer Mar 1, 2019

Uh oh!

biancadanforth Mar 5, 2019

Uh oh!

rhelmer Mar 1, 2019

Uh oh!

biancadanforth Mar 5, 2019

Uh oh!

rhelmer Mar 1, 2019

Uh oh!

biancadanforth commented Mar 5, 2019

Uh oh!

rhelmer commented Mar 5, 2019 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Create test WE for Bug 1292234 #3

Are you sure you want to change the base?

Create test WE for Bug 1292234 #3

Uh oh!

Conversation

biancadanforth commented Mar 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rhelmer commented Mar 1, 2019

Uh oh!

rhelmer left a comment

Choose a reason for hiding this comment

Uh oh!

rhelmer Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

biancadanforth Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

rhelmer Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

biancadanforth Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

rhelmer Mar 1, 2019

Choose a reason for hiding this comment

Uh oh!

biancadanforth commented Mar 5, 2019

Uh oh!

rhelmer commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

biancadanforth commented Mar 1, 2019 •

edited

Loading

rhelmer commented Mar 5, 2019 •

edited

Loading