1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect, P2)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

STR:

1. Open some chrome:// page, such as chrome://devtools/content/responsive.html/index.xhtml
2. Open the toolbox and go to the Storage tool
3. The storage types on the left side are missing, and there are several errors in the Browser Console:

14:09:31.940 Exception { message: "Component returned failure code: 0x…", result: 2147500037, name: "NS_ERROR_FAILURE", filename: "resource://gre/modules/commonjs/too…", lineNumber: 1110, columnNumber: 0, data: null, stack: ".getCachesForHost<@resource://gre/m…", location: XPCWrappedNative_NoHelper }protocol.js:907
14:09:31.937 Invalid chrome URI: /
14:09:31.949 Protocol error (unknownError): Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIIOService2.newURI]

ER:

I should be able to see storage used by the chrome:// page.

AR:

The storage types are missing entirely, so I can't see any storage details.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

  Message: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIIOService2.newURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/storage.js :: .getCachesForHost< :: line 1123"  data: no]
  Stack:
    .getCachesForHost<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/storage.js:1123:15
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
.populateStoresForHost<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/storage.js:1203:24
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
.preListStores<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/storage.js:1140:13
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
StorageActor<.listStores<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/storage.js:2214:15
TaskImpl.prototype._run@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:311:39
TaskImpl@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:273:3
createAsyncFunction/asyncFunction@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/task.js:247:14
generateRequestHandlers/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/protocol.js:1047:19
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1648:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:13
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14

Filter under Floccinaucinihilipilification

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Having done a lot of investigation it seems that cookies, localStorage and sessionStorage are unavailable outside of the content process.

I am not sure if the DOM cache is available.

I have a patch that prevents the storage panel from crashing but need to create a web extension with indexedDB and possibly DOM cache running in a background script.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

localStorage and sessionStorage only works for URIs with hostnames, and chrome:// URIs don't have hostnames.

We also have a test that chrome pages are not able to access window.localStorage/sessionStorage objects:
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/localstorage/test_localStorageFromChrome.xhtml (and similar for sessionStorage).

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API/Using_IndexedDB_in_chrome

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [WIP] - Storage inspector doesn't work on chrome:// pages

This patch allows chrome pages and web extension's indexedDBs to be viewed in the storage inspector.

As it turns out there are multiple issues with indexedDB. We never paid attention to whether the data was temporary or persistent which led to us failing to discover or edit various databases. This patch addresses this issue and fixes multiple bugs.

I still need to finish the indexedDB side and ensure that the other storage types are properly discovered.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Turns out that the DOM cache is *only* available to use in https pages so it will not work on chrome:// pages or inside web extensions.

This means that only indexedDB data is available in this situation.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [WIP] - Storage inspector doesn't work on chrome:// pages

Improving indexed discovery.

Still need to ensure we abort searching for other storage types on pages they are not available on.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

It appears that multiple databases of the same name can exist for the same page... one for each storage type. This means that name is no longer unique so I need to make changes to the indexedDB storage actor in order to address this.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: [WIP] - Storage inspector doesn't work on chrome:// pages

Just need a rebase and try before fixing tests and review.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aabb9bb0b8cd

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Storage inspector doesn't work on chrome:// pages and web extensions

- Rebased on top of cookie name patch.
- Fixed tests and a couple of bugs.
- Began writing new tests.

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

Changes:
  - Added StopIteration to .eslintrc globals (it is a standard JS global).
  - Changed indexedDBs "Key" column to "Key Path."
  - Added simple test.
  - Renamed database throughout tests as we display the database name with the storage type in brackets e.g. "idb1 (default), idb1 (temporary) or idb1 (persistent)"
  - Fixed database discovery to take into account the different storage types.
  - Fixed various crashers that prevented the storage inspector working in chrome:// or Web Extension context.
  - I will attach a web extension for testing to the bug... just install it from about:debugging.

Review commit: https://reviewboard.mozilla.org/r/95380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95380/

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: indexedDB WebExtension.zip

This web extension can be used for testing.

Comment 14

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Tests run fine locally... i'll try chaos mode to see what is causing the failures.

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Test failures are in Linux and Windows 7 only.

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8814067 [details]
Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95380/diff/1-2/

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8814067 [details]
Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

https://reviewboard.mozilla.org/r/95380/#review95560

Thanks for the patch Mike! It does fix the storage inspector in chrome:// URLs
If I understand correctly this contains at least two distinct parts:
- support for chrome:// URLs
- support for storageType for indexedDB
(+ various fixes)

Could we split this in two commits, and maybe bugs?

I feel like we should think a bit more about the way we display the storage type.
Adding it in the name might be confusing for users, if they don't know where this is coming from.
What about a new column "storage type" to display this information?

::: devtools/.eslintrc.js:30
(Diff revision 1)
>      "Node": true,
>      "reportError": true,
>      "require": true,
>      "setInterval": true,
>      "setTimeout": true,
> +    "StopIteration": true,

I think StopIteration should remain explicitly opt-in. It's non-standard (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/StopIteration), won't work in the scope of devtools.html

::: devtools/server/actors/storage.js:1444
(Diff revision 1)
>   * @param {string} origin
>   *        The host associated with this indexed db.
>   * @param {IDBDatabase} db
>   *        The particular indexed db.
>   */
> -function DatabaseMetadata(origin, db) {
> +function DatabaseMetadata(origin, db, storage) {

nit: add missing jsdoc

::: devtools/server/actors/storage.js:1741
(Diff revision 2)
>      if (!DebuggerServer.isInChildProcess) {
>        this.backToChild = (func, rv) => rv;
> +      this.clearDBStore = indexedDBHelpers.clearDBStore;
> +      this.gatherFilesOrFolders = indexedDBHelpers.gatherFilesOrFolders;
>        this.getDBMetaData = indexedDBHelpers.getDBMetaData;
> -      this.openWithPrincipal = indexedDBHelpers.openWithPrincipal;
> +      this.splitNameAndStorage = indexedDBHelpers.splitNameAndStorage;

seems like this list is sorted alphabetically, so we should move this line down.

::: devtools/server/actors/storage.js:2188
(Diff revision 2)
> -   *        ofsset of the entries to be fetched.
> +   *        offset of the entries to be fetched.
>     * @param {number} size
>     *        The intended size of the entries to be fetched.
>     */
>    getObjectStoreData(host, principal, dbName, objectStore, id, index,
> -                     offset, size) {
> +                     offset, size, storage) {

nit: missing jsdoc for storage

On a sidenote, the arguments list for this method starts to be hard to understand.

Maybe it would be easier to regroup the request options as follows:
- host
- principal
- dbName
- storage
- requestOptions: 
  - id
  - index 
  - offset
  - size

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8814067 [details]
Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

https://reviewboard.mozilla.org/r/95380/#review95560

A have received so many requests to fix this that I want to land it as soon as possible.

I don't really want to split this because support for chrome:// URLs is useless without storageType (the storage type is usually "persistent" from about pages and other chrome URLs).

I agree that we shoult split the storage type into a new column. This means creating compound keys and a lot of test changes so I have logged bug 1320362 for that.

> I think StopIteration should remain explicitly opt-in. It's non-standard (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/StopIteration), won't work in the scope of devtools.html

Agreed

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8814067 [details]
Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95380/diff/2-3/

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Now windows only issues... my windows box is angry at the moment so I will push to try with extra logging to fix this one.

Comment 21

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=919561b57a9b

Comment 22

[Julian Descottes [:jdescottes]]

Comment on attachment 8814067 [details]
Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

https://reviewboard.mozilla.org/r/95380/#review95972

Regarding the windows test failures, I can't manage to displayed any indexedDB information on Windows (and haven't had a great success debugging it so far). This probably explains the try failures on Windows.

I have seen error logs for:
- console.warn(`Failed to enumerate ${type} for host ${host}: ${ex}`);
- let parsedName = JSON.parse(name); (in getObjectsSize)

But for some reason when want to actually debug it, I can't get a breakpoint or an error.

Let me know if you want me to have a deeper look into the Windows issue. If you can't get you windows box back in shape, it might be a bit painful to just investigate with try. 

I'm clearing the review flag until the windows issue is resolved, in case there is a significant code change. Otherwise just a few nits, but looks good.

Regarding our discussion about splitting the bug in two, I won't block the review on this. I understand you prefer to land this asap and you need to access the persistent storage DBs to have a proper suppport for chrome:// urls here. I'm still not sold on surfacing this to the UI in its current state, but if it makes it easier to implement for now let's go for this.

Would be really great if we could prioritize your follow up bug and attempt to land it in 53 too.

::: devtools/server/actors/storage.js:2014
(Diff revision 3)
> +    let storagePath = OS.Path.join(profileDir, "storage");
> +    let sqliteFiles =
> +      yield this.gatherFilesOrFolders(storagePath, /\/idb\/[^/]+\.sqlite$/);
>  
> -    let exists = yield OS.File.exists(directory);
> -    if (!exists && host.startsWith("about:")) {
> +    for (let file of sqliteFiles) {
> +      let splitPath = OS.Path.split(file).components;

A small comment here explaining the expected skeleton of the sqlite file path would be great.

::: devtools/server/actors/storage.js:2041
(Diff revision 3)
> -    }
> +        }
> +      }
> -    return this.backToChild("getDBNamesForHost", {names: names});
> +      return this.backToChild("getDBNamesForHost", {names: names});
> +    }
> +
> +    return this.backToChild("getDBNamesForHost", {names: []});

nit: we could remove the return from line 2038 in the if block and always 
> return this.backToChild("getDBNamesForHost", {names});

if files.length === 0, names will be [] anyway.

::: devtools/server/actors/storage.js:2057
(Diff revision 3)
> +
> +        for (let child in iterator) {
> +          child = yield child;
> +
> +          // We need to predeclare these variables *before* the condition to
> +          // prevent

Feels like we are missing the end of the comment here

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cd4278fd5f9

Comment 24

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8814067 [details]
Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

https://reviewboard.mozilla.org/r/95380/#review95972

I couldn't agree more... I will add the storage column and compound keys next.

> A small comment here explaining the expected skeleton of the sqlite file path would be great.

// We expect sqlite DB paths to look something like this:
    // - PathToProfileDir/storage/default/http+++www.example.com/
    //   idb/1556056096MeysDaabta.sqlite
    // - PathToProfileDir/storage/permanent/http+++www.example.com/
    //   idb/1556056096MeysDaabta.sqlite
    // - PathToProfileDir/storage/temporary/http+++www.example.com/
    //   idb/1556056096MeysDaabta.sqlite
    //
    // The subdirectory inside the storage folder is determined by the storage
    // type:
    // - default:   { storage: "default" } or not specified.
    // - permanent: { storage: "persistent" }.
    // - temporary: { storage: "temporary" }.
    
    We have stopped using a regex to detect sqlite files from paths and use the following method:
    
    ```
    let sqliteFiles = yield this.gatherFilesOrFolders(storagePath, path => {
      if (path.endsWith(".sqlite")) {
        let { components } = OS.Path.split(path);
        let isIDB = components[components.length - 2] === "idb";

        return isIDB;
      }
      return false;
    });
    ```
    
    Where storagePath is "profileDir/storage/" and the second parameter is a validation function. This is far less error prone than using regular expressions.

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8814067 [details]
Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95380/diff/3-4/

Comment 26

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8814067 [details]
Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

https://reviewboard.mozilla.org/r/95380/#review96430

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8814067 [details]
Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95380/diff/4-5/

Comment 28

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8814067 [details]
Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

https://reviewboard.mozilla.org/r/95378/#review96432

Comment 29

[Julian Descottes [:jdescottes]]

Comment on attachment 8814067 [details]
Bug 1276339 - Storage inspector doesn't work on chrome:// pages and web extensions

https://reviewboard.mozilla.org/r/95380/#review96436

Looks good and works just fine on Windows now. 
Thanks!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cdeaf29cac0
Storage inspector doesn't work on chrome:// pages and web extensions r=jdescottes

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8cdeaf29cac0