Closed Bug 1445160 Opened 6 years ago Closed 6 years ago

Cannot inspect indexedDBs inside JSMs using the storage inspector in the browser toolbox

Categories

(DevTools :: Storage Inspector, defect, P2)

Product:
DevTools
Component:
Storage Inspector
Type:
defect

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(1 file)

Bug 1445160 - Enable inspect indexedDBs inside JSMs via Storage Inspector in browser toolbox
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Has Regression Range: --- → irrelevant
Has STR: --- → yes
Summary: IndexedDB not available when debugging the main browser process → about: pages not available from the browser toolbox storage inspector
Summary: about: pages not available from the browser toolbox storage inspector → Cannot inspect indexedDBs inside JSMs using the storage inspector
STR:

1. Open about:home
2. Press [cmd][alt][k] to open the web console.
3. Run gSnippetsMap.blockSnippetById(1) in the web console.
4. To ensure it has taken run gSnippetsMap.blockList

The code from here:
https://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/ActivityStreamStorage.jsm#31

Will have created a DB here:
~/Library/Application Support/Firefox/Profiles/27bxqsvk.fromsrc/storage/permanent/chrome/idb/1657114595AmcateirvtiSty.sqlite

That DB is not available in the Storage Inspector because it is not linked with a URL as web-based indexedDBs are.

We need to find a way to list indexedDBs from this folder inside the browser toolbox.
Summary: Cannot inspect indexedDBs inside JSMs using the storage inspector → Cannot inspect indexedDBs inside JSMs using the storage inspector in the browser toolbox
We can ensure we are in the browser toolbox by checking target.chrome.
Unfortunately, it is not possible to test tools inside the browser console and because this feature is only enabled in the browser console it is not possible to test it using our automated testing system.

Manual testing instructions:

1. Open about:home
2. Press [cmd][alt][k] to open the web console.
3. Run gSnippetsMap.blockSnippetById(1) in the web console.
4. To ensure it has taken run gSnippetsMap.blockList
5. Open the browser toolbox.
6. The following path should be available: "IndexedDB" > "chrome" > "ActivityStream (persistent)" > "snippets".
7. When "snippets" is selected the Key should be "blockList" and the Value should be "[1]"
8. Close the browser toolbox.
9. Open the default toolbox.
10. The path should not exist.
Comment on attachment 8961004 [details]
Bug 1445160 - Enable inspect indexedDBs inside JSMs via Storage Inspector in browser toolbox

https://reviewboard.mozilla.org/r/229720/#review235824

I have some nits and a question.
Also, regarding tests, if we can't have mochitests could we at least try to unit test the functions involved here ?
We need to have something tracking that this feature does not break IMO.

::: devtools/client/storage/ui.js:146
(Diff revision 1)
> +
> +        for (let host in hosts) {
> +          if (!host.startsWith("http://") &&
> +              !host.startsWith("https://") &&
> +              !host.startsWith("about:")) {
> +            delete hosts[host];

I always hear `delete` being slow, could we use something else here ?

If hosts is an array, let's use filter. If it's an object. let's use reduce on Object.entries() to recreate a new host.

nit: could we create a `SAFE_HOSTS_PREFIXES = ["http://", "https://", "about:"]` ? That might be a bit simpler to handle.

::: devtools/server/actors/storage.js:358
(Diff revision 1)
>          // because the principal only matters the indexedDB.
>          let win = this.storageActor.getWindowFromHost(host);
>          if (win) {
>            principal = win.document.nodePrincipal;
> +        } else {
> +          // We are running in the browser console and viewing system DBs so we

nit: replace "console" by "toolbox"

::: devtools/server/actors/storage.js:1640
(Diff revision 1)
> +    this._internalHosts = await this.getInternalHosts();
> +
> +    return this.hosts;

this is a bit confising to me, we populate `this._internalHosts` but return `this.hosts`, is it wanted ?

If it is, could we refactor this or use better naming to explain what is going on ?

::: devtools/server/actors/storage.js:1773
(Diff revision 1)
>      if (win) {
> -      let principal = win.document.nodePrincipal;
> +      principal = win.document.nodePrincipal;
> +    } else {
> +      // We are running in the browser console and viewing system DBs so we
> +      // need to use system principal.
> +      principal = Cc["@mozilla.org/systemprincipal;1"]
> +                    .createInstance(Ci.nsIPrincipal);
> +    }

that's the second time we have this if/else statement, maybe we could extract it in a getPrincipal function ?

```js
getPrincipal(win) {
  return win
    ? win.document.nodePrincipal
    : Cc["@mozilla.org/systemprincipal;1"] .createInstance(Ci.nsIPrincipal);
}
```

::: devtools/server/actors/storage.js:2049
(Diff revision 1)
> +          !entry.name.startsWith("about+") &&
> +          !entry.name.startsWith("http+++") &&
> +          !entry.name.startsWith("https+++")) {

I guess we could re-use the `SAFE_HOSTS_PREFIXES` array with some tweaks here ?
Attachment #8961004 - Flags: review?(nchevobbe) → review-
Comment on attachment 8961004 [details]
Bug 1445160 - Enable inspect indexedDBs inside JSMs via Storage Inspector in browser toolbox

https://reviewboard.mozilla.org/r/229720/#review235844

::: devtools/client/storage/ui.js:146
(Diff revision 1)
> +
> +        for (let host in hosts) {
> +          if (!host.startsWith("http://") &&
> +              !host.startsWith("https://") &&
> +              !host.startsWith("about:")) {
> +            delete hosts[host];

delete is slow but this object will only ever be iterated over once and will very rarely have more than five entries.

Even so, to make you happy I have switched to using a regex and generating a new object.

::: devtools/server/actors/storage.js:1640
(Diff revision 1)
> +    this._internalHosts = await this.getInternalHosts();
> +
> +    return this.hosts;

The naming is fine but I have added a comment explaining why we need to do it this way:

```
// Add internal hosts to this._internalHosts, which will be picked up by
// the this.hosts getter. Because this.hosts is a property on the default
// storage actor and inherited by all storage actors we have to do it this
// way.
```
Comment on attachment 8961004 [details]
Bug 1445160 - Enable inspect indexedDBs inside JSMs via Storage Inspector in browser toolbox

https://reviewboard.mozilla.org/r/229720/#review235824

Bug 1448118 created to try to find a way to test the storage inspector inside the browser toolbox or fool the actor into believing we are running inside the browser toolbox.
Comment on attachment 8961004 [details]
Bug 1445160 - Enable inspect indexedDBs inside JSMs via Storage Inspector in browser toolbox

https://reviewboard.mozilla.org/r/229720/#review236018

Thanks Mike.
Hope we'll find something for testing this
Attachment #8961004 - Flags: review?(nchevobbe) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)
> Comment on attachment 8961004 [details]
> Bug 1445160 - Enable inspect indexedDBs inside JSMs via Storage Inspector in
> browser toolbox
> 
> https://reviewboard.mozilla.org/r/229720/#review236018
> 
> Thanks Mike.
> Hope we'll find something for testing this

Don't worry, I will try to land this when I have a test... I have a plan.
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f41a708dce5f
Enable inspect indexedDBs inside JSMs via Storage Inspector in browser toolbox r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/f41a708dce5f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: