Closed Bug 1276339 Opened 8 years ago Closed 7 years ago

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

Categories

(DevTools :: Storage Inspector, defect, P2)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: jryans, Assigned: miker)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
  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
Priority: -- → P2
Assignee: nobody → mratcliffe
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.
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).
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.
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.
Improving indexed discovery.

Still need to ensure we abort searching for other storage types on pages they are not available on.
Attachment #8811344 - Attachment is obsolete: true
Summary: Storage inspector doesn't work on chrome:// pages → Storage inspector doesn't work on chrome:// pages and web extensions
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.
Just need a rebase and try before fixing tests and review.
Attachment #8811837 - Attachment is obsolete: true
- Rebased on top of cookie name patch.
- Fixed tests and a couple of bugs.
- Began writing new tests.
Attachment #8812239 - Attachment is obsolete: true
Attachment #8813774 - Attachment is obsolete: true
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/
Attachment #8814067 - Flags: review?(jdescottes)
This web extension can be used for testing.
Tests run fine locally... i'll try chaos mode to see what is causing the failures.
Test failures are in Linux and Windows 7 only.
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
Attachment #8814067 - Flags: review?(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

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
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 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
Attachment #8814067 - Flags: review?(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

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 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 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 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!
Attachment #8814067 - Flags: review?(jdescottes) → review+
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8cdeaf29cac0
Storage inspector doesn't work on chrome:// pages and web extensions r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/8cdeaf29cac0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: