Closed Bug 1448118 Opened 6 years ago Closed 6 years ago

Find a way to test the storage inspector inside the browser toolbox

Categories

(DevTools :: Storage Inspector, enhancement, P2)

Product:
DevTools
Component:
Storage Inspector
Type:
enhancement

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 1448118 - Test for the storage inspector when running inside the browser toolbox
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
We need to create a test for bug 1445160
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
I believe :ochameau has written a few Browser Toolbox tests, such as

https://searchfox.org/mozilla-central/source/devtools/client/framework/test/browser_browser_toolbox_debugger.js

for testing the debugger in Browser Toolbox.
Comment on attachment 8961953 [details]
Bug 1448118 - Test for the storage inspector when running inside the browser toolbox

https://reviewboard.mozilla.org/r/230792/#review236598

::: devtools/server/tests/browser/browser_storage_browser_toolbox_indexeddb.js:1
(Diff revision 5)
> +/* vim: set ft=javascript ts=2 et sw=2 tw=80: */

nit: I tend not to include those in new files

::: devtools/server/tests/browser/browser_storage_browser_toolbox_indexeddb.js:13
(Diff revision 5)
> +  await new Promise(resolve => {
> +    let options = {"set": [
> +      ["devtools.chrome.enabled", true],
> +      ["devtools.debugger.remote-enabled", true],
> +    ]};
> +    SpecialPowers.pushPrefEnv(options, resolve);
> +  });

I think you can use pushPref (https://searchfox.org/mozilla-central/rev/003262ae12ce937950ffb8d3b0fa520d1cc38bff/devtools/client/shared/test/shared-head.js#563) for that

::: devtools/server/tests/browser/browser_storage_browser_toolbox_indexeddb.js:33
(Diff revision 5)
> +  forceCollections();
> +  await client.close();
> +  forceCollections();
> +  DebuggerServer.destroy();
> +  forceCollections();

do we need to forceCollections 3 times ? Can't we just call it at the end ? (i'm sure you already tested this, but I'm curious what's going on)

::: devtools/server/tests/browser/browser_storage_browser_toolbox_indexeddb.js:49
(Diff revision 5)
> +    let db = request.result;
> +    let store = db.createObjectStore("MyObjectStore", {keyPath: "id"});
> +    store.createIndex("NameIndex", ["name.last", "name.first"]);
> +  };
> +
> +  await undefined;

what is this line for ?
Comment on attachment 8961953 [details]
Bug 1448118 - Test for the storage inspector when running inside the browser toolbox

clearing review flag
Attachment #8961953 - Flags: review?(nchevobbe)
Comment on attachment 8961953 [details]
Bug 1448118 - Test for the storage inspector when running inside the browser toolbox

https://reviewboard.mozilla.org/r/230792/#review239302

::: devtools/server/tests/browser/browser_storage_browser_toolbox_indexeddb.js:33
(Diff revision 5)
> +  forceCollections();
> +  await client.close();
> +  forceCollections();
> +  DebuggerServer.destroy();
> +  forceCollections();

Yeah, we had lots of issues with leaks in these files... quite a few of the tests are forced to do the same thing.

::: devtools/server/tests/browser/browser_storage_browser_toolbox_indexeddb.js:49
(Diff revision 5)
> +    let db = request.result;
> +    let store = db.createObjectStore("MyObjectStore", {keyPath: "id"});
> +    store.createIndex("NameIndex", ["name.last", "name.first"]);
> +  };
> +
> +  await undefined;

It is the standard way to wait for the indexedDB to be completed in an async function.

await undefined creates a new promise and returns undefined... it is done this way all over the fx codebase.
Comment on attachment 8961953 [details]
Bug 1448118 - Test for the storage inspector when running inside the browser toolbox

https://reviewboard.mozilla.org/r/230792/#review239302

> It is the standard way to wait for the indexedDB to be completed in an async function.
> 
> await undefined creates a new promise and returns undefined... it is done this way all over the fx codebase.

could this cause a race condition ? I guess it works because we rely on the fact that the database is created fast enough. 
Would the following work ?

```js
let request = indexedDB.open("MyDatabase", 1);
await new Promise(resolve => {
  request.onupgradeneeded = function() {
    let db = request.result;
    let store = db.createObjectStore("MyObjectStore", {keyPath: "id"});
    store.createIndex("NameIndex", ["name.last", "name.first"]);
    resolve();
  };
});
```
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #11)
> Comment on attachment 8961953 [details]
> Bug 1448118 - Test for the storage inspector when running inside the browser
> toolbox
> 
> https://reviewboard.mozilla.org/r/230792/#review239302
> 
> > It is the standard way to wait for the indexedDB to be completed in an async function.
> > 
> > await undefined creates a new promise and returns undefined... it is done this way all over the fx codebase.
> 
> could this cause a race condition ? I guess it works because we rely on the
> fact that the database is created fast enough. 
> Would the following work ?
> 
> ```js
> let request = indexedDB.open("MyDatabase", 1);
> await new Promise(resolve => {
>   request.onupgradeneeded = function() {
>     let db = request.result;
>     let store = db.createObjectStore("MyObjectStore", {keyPath: "id"});
>     store.createIndex("NameIndex", ["name.last", "name.first"]);
>     resolve();
>   };
> });
> ```

To be honest I don't see the problem... It absolutely cannot cause a race condition because when you call await, the currently running call stack, up to the point of await, is allowed to finish so that other functions in the callback queue can be executed. This means that await undefined will never be executed until the indexedDB is ready.

You do the same thing when running indexedDB transactions.

This is the standard way of working with indexedDBs inside async functions, see:
https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/test_filehandle_store_snapshot.html
https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/error_events_abort_transactions_iframe.html

Or even see the same in Chrome's source:
https://chromium.googlesource.com/external/github.com/mozilla/gecko-dev/+/B2G_2_2_20151214_MERGEDAY/browser/devtools/storage/test/storage-complex-values.html

Like I say, `await undefined` creates a new promise and returns undefined... it is done this way all over the fx codebase:
https://searchfox.org/mozilla-central/search?q=yield+undefined&path=
Comment on attachment 8961953 [details]
Bug 1448118 - Test for the storage inspector when running inside the browser toolbox

https://reviewboard.mozilla.org/r/230792/#review239302

> could this cause a race condition ? I guess it works because we rely on the fact that the database is created fast enough. 
> Would the following work ?
> 
> ```js
> let request = indexedDB.open("MyDatabase", 1);
> await new Promise(resolve => {
>   request.onupgradeneeded = function() {
>     let db = request.result;
>     let store = db.createObjectStore("MyObjectStore", {keyPath: "id"});
>     store.createIndex("NameIndex", ["name.last", "name.first"]);
>     resolve();
>   };
> });
> ```

To be honest I don't see the problem... It absolutely cannot cause a race condition because when you call await, the currently running call stack, up to the point of await, is allowed to finish so that other functions in the callback queue can be executed. This means that await undefined will never be executed until the indexedDB is ready.

You do the same thing when running indexedDB transactions.

This is the standard way of working with indexedDBs inside async functions, see:
https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/test_filehandle_store_snapshot.html
https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/error_events_abort_transactions_iframe.html

Or even see the same in Chrome's source:
https://chromium.googlesource.com/external/github.com/mozilla/gecko-dev/+/B2G_2_2_20151214_MERGEDAY/browser/devtools/storage/test/storage-complex-values.html

Like I say, `await undefined` creates a new promise and returns undefined... it is done this way all over the fx codebase:
https://searchfox.org/mozilla-central/search?q=yield+undefined&path=
Thanks for the answers Mike, I'm not used to IndexedDB and I may be confused by how it seems to be async but can't really be longer than a tick ? Anyway, this is consistent with the rest of the codebase so I'm fine with this, I was just curious :)
Comment on attachment 8961953 [details]
Bug 1448118 - Test for the storage inspector when running inside the browser toolbox

https://reviewboard.mozilla.org/r/230792/#review239336
Attachment #8961953 - Flags: review?(nchevobbe) → review+
Comment on attachment 8961953 [details]
Bug 1448118 - Test for the storage inspector when running inside the browser toolbox

https://reviewboard.mozilla.org/r/230792/#review240944


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/server/tests/browser/browser_storage_browser_toolbox_indexeddb.js:15
(Diff revision 7)
> +  this);
> +
> +add_task(async function() {
> +  await pushPref("devtools.chrome.enabled", true);
> +  await pushPref("devtools.debugger.remote-enabled", true);
> +  await openTabAndSetupStorage(MAIN_DOMAIN + "storage-updates.html");

Error: 'opentabandsetupstorage' is not defined. [eslint: no-undef]

::: devtools/server/tests/browser/browser_storage_browser_toolbox_indexeddb.js:24
(Diff revision 7)
> +  let client = new DebuggerClient(DebuggerServer.connectPipe());
> +  let form = await connectDebuggerClient(client);
> +  let front = StorageFront(client, form);
> +
> +  await testInternalDBs(front);
> +  await clearStorage();

Error: 'clearstorage' is not defined. [eslint: no-undef]
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14fa3a0b82e2
Test for the storage inspector when running inside the browser toolbox r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/14fa3a0b82e2
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: