robinkotter Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160725105554

Steps to reproduce:

I created an angularJS app and use the $routeProvider for templating.
so the links are like:
<a href="#/">Start</a> | <a href="#/about">About</a
default in the url is now:
angular.html#/ or angular.html#/about

on a click function i just added some test code to see how the indexedDB is working:



// This works on all devices/browsers, and uses IndexedDBShim as a final fallback 
var indexedDB = window.indexedDB || window.mozIndexedDB || window.webkitIndexedDB || window.msIndexedDB || window.shimIndexedDB;

// Open (or create) the database
var open = indexedDB.open("MyDatabase", 1);

// Create the schema
open.onupgradeneeded = function() {
    var db = open.result;
    var store = db.createObjectStore("MyObjectStore", {keyPath: "id"});
    var index = store.createIndex("NameIndex", ["name.last", "name.first"]);
};

open.onsuccess = function() {
    // Start a new transaction
    var db = open.result;
    var tx = db.transaction("MyObjectStore", "readwrite");
    var store = tx.objectStore("MyObjectStore");
    var index = store.index("NameIndex");

    // Add some data
    store.put({id: 12345, name: {first: "John", last: "Doe"}, age: 42});
    store.put({id: 67890, name: {first: "Bob", last: "Smith"}, age: 35});
    
    // Query the data
    var getJohn = store.get(12345);
    var getBob = index.get(["Smith", "Bob"]);

    getJohn.onsuccess = function() {
        console.log(getJohn.result.name.first);  // => "John"
    };

    getBob.onsuccess = function() {
        console.log(getBob.result.name.first);   // => "Bob"
    };

    // Close the db when the transaction is done
    tx.oncomplete = function() {
        db.close();
    };
}


Actual results:

the developer tools say "no data for the choosen host aviable "

if I put the code in a normal html file everything works fine
according to this stack overflow post:
http://stackoverflow.com/a/30356990/2044537
there seems to be a problem with the hashtag in the url


Expected results:

I expected, that everything works normally ;)

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 1 • 8 years ago

Creating an indexedDB under a file:/// path includes everything after a # for a domain. So going to the following URLs and creating a DB will create three completely separate DBs:

  - file:///some/path/test.html
  - file:///some/path/test.html#test
  - file:///some/path/test.html#/test

The DBs are created as:

  - file++++some+path+test.html
  - file++++some+path+test.html#test
  - file++++some+path+test.html#+test

Chrome and Safari instead group all file:/// indexedDBs into one location with a hostname of file__0. I think our approach is better but surely we should trim the host at the first #?

I have logged bug 1321550 to investigate this.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 2 • 8 years ago

Changes:
  -  Added storage-listings-with-fragment.html and browser_storage-listings-with-fragment.js. The only difference between these and storage-listings.html and browser_storage-listings.js is that they contain url fragments wherever URLs are loaded e.g. #abc, #def etc.
  - When referencing cookies in tests we used to only use the hostname but a full URL was needed for other storage types. For consistency we now use the full URL for all storage types.
  - storage.js used to contain various getHostName() methods depending on which storage type was needed. This has been changed to a single method that acts according to which protocol is in use. Cookies are the only storage type that requires just a hostname for the http and https protocols so we strip them inside the cookies actor where required. null is returned when storage types are not available for a particular protocol e.g. data:// URLs.
  - browser_storage_dynamic_windows.js and browser_storage_listings.js now detect cookies that were previously missed. This is a result of the getHostName() improvements.

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

	Nicolas Chevobbe [:nchevobbe]
	Comment 3 • 8 years ago
mozreview-review

Comment on attachment 8826167 [details]
Bug 1302989 - Make storage inspector work with file:// when # is in the URL

https://reviewboard.mozilla.org/r/104190/#review105088

::: devtools/client/storage/test/browser_storage_basic_with_fragment.js:94
(Diff revision 1)
> +  for (let item of testCases) {
> +    ok(doc.querySelector("[data-id='" + JSON.stringify(item[0]) + "']"),
> +       "Tree item " + item[0] + " should be present in the storage tree");

Maybe we could have `for { let [treeItem] of testCases }` instead of accessing `item[0]`

::: devtools/client/storage/test/browser_storage_basic_with_fragment.js:115
(Diff revision 1)
> +    ok(doc.querySelector(".table-widget-cell[data-id='" + id + "']"),
> +       "Table item " + id + " should be present");
> +  }
> +
> +  // Click rest of the tree items and wait for the table to be updated
> +  for (let item of testCases.slice(1)) {

We also could use destructuring here `for (let [treeItem, items] of ...`

::: devtools/client/storage/test/browser_storage_basic_with_fragment.js:132
(Diff revision 1)
> +  yield openTabAndSetupStorage(MAIN_DOMAIN +
> +                               "storage-listings-with-fragment.html#abc");

What would you think of something like :
```
yield openTabAndSetupStorage(
  MAIN_DOMAIN + "storage-listings-with-fragment.html#abc");
```

::: devtools/client/storage/test/storage-listings-with-fragment.html:4
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +Bug 970517 - Storage inspector front end - tests

Is this bug number relevant ? I see that it is fixed for almost a year now

::: devtools/client/storage/test/storage-listings-with-fragment.html:8
(Diff revision 1)
> +<!--
> +Bug 970517 - Storage inspector front end - tests
> +-->
> +<head>
> +  <meta charset="utf-8">
> +  <title>Storage inspector test for listing hosts and storages</title>

maybe add something about fragment ?

::: devtools/client/storage/test/storage-listings-with-fragment.html:36
(Diff revision 1)
> +  let db = await new Promise(done => {
> +    request.onupgradeneeded = event => {
> +      let db = event.target.result;
> +      let store1 = db.createObjectStore("obj1", { keyPath: "id" });
> +      store1.createIndex("name", "name", { unique: false });
> +      store1.createIndex("email", "email", { unique: true });
> +      let store2 = db.createObjectStore("obj2", { keyPath: "id2" });
> +      store1.transaction.oncomplete = () => {
> +        done(db);
> +      };
> +    };
> +  });
> +
> +  // Prevents AbortError
> +  await new Promise(done => {
> +    request.onsuccess = done;
> +  });

Could this be a cause of race condition ? I mean, could it be possible that the onsuccess event is fired before this Promise is set up ?

Something that bug me a bit here is that we're waiting on something without doing any work, we're just setting up a listener, so I guess we depend on a previous call.

We could have the 2 same Promises, and then await on `Promise.all[onUpgradeNedded, onSuccess]` for example

I'm not familiar with indexedDB so please tell me if I'm wrong.

::: devtools/client/storage/test/storage-listings-with-fragment.html:67
(Diff revision 1)
> +  await new Promise(success => {
> +    transaction.oncomplete = success;
> +  });

Same here, could the oncomplete event be fired before we set up this promise ? Maybe we could declare the promise on a onComplete variable, add the items and then await.
Again, I can be wrong since I'm not familiar with indexedDB

::: devtools/client/storage/test/storage-listings-with-fragment.html:111
(Diff revision 1)
> +  let cache = await caches.open("plop");
> +  await fetchPut(cache, "404_cached_file.js");
> +  await fetchPut(cache, "browser_storage_basic.js");
> +};
> +
> +window.setup = function*() {

can't this be an async function ?

::: devtools/client/storage/test/storage-listings-with-fragment.html:119
(Diff revision 1)
> +  if (window.caches) {
> +    yield cacheGenerator();
> +  }
> +};
> +
> +window.clear = function*() {

can't this be an async function ?

::: devtools/server/actors/storage.js:771
(Diff revision 1)
> +  trimHttpHttps(url) {
> +    if (url.startsWith("http://")) {
> +      return url.substr(7);
> +    }
> +    if (url.startsWith("https://")) {
> +      return url.substr(8);
> +    }
> +    return url;
> +  },

isn't this function the same one as the one created before ?  
If we can't access the one created before, maybe we could the method to a function (i.e. not as a member of an object) at the bottom of the file (or in a util file if we already have such things)

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 4 • 8 years ago
mozreview-review-reply

Comment on attachment 8826167 [details]
Bug 1302989 - Make storage inspector work with file:// when # is in the URL

https://reviewboard.mozilla.org/r/104190/#review105088

I am sorry to have dismissed a bunch of your comments about the storage tests but all of these tests need to be completely rewritten when we convert the storage inspector to use React in the near future so there is no point me rewriting them although I agree that they are inconsistent and confusing.

> We also could use destructuring here `for (let [treeItem, items] of ...`

I was copying browser_storage_basic.js here so I will copy these changes to both files.

> What would you think of something like :
> ```
> yield openTabAndSetupStorage(
>   MAIN_DOMAIN + "storage-listings-with-fragment.html#abc");
> ```

Done

> Could this be a cause of race condition ? I mean, could it be possible that the onsuccess event is fired before this Promise is set up ?
> 
> Something that bug me a bit here is that we're waiting on something without doing any work, we're just setting up a listener, so I guess we depend on a previous call.
> 
> We could have the 2 same Promises, and then await on `Promise.all[onUpgradeNedded, onSuccess]` for example
> 
> I'm not familiar with indexedDB so please tell me if I'm wrong.

No chance of a race condition... indexedDB is far too slow for that.

Ugly though it is, this is a **really** common pattern in indexedDB code. Identical code is used e.g. in almost every html file in this test folder.

Rather than rewrite every storage inspector test let's leave it as it is for now.

> Same here, could the oncomplete event be fired before we set up this promise ? Maybe we could declare the promise on a onComplete variable, add the items and then await.
> Again, I can be wrong since I'm not familiar with indexedDB

See previous reply.

> can't this be an async function ?

We could but that require making changes to head.js and every test in this folder.

Considering that the tests will be rewritten when we convert the storage inspector to use react it is not worth doing this.

> can't this be an async function ?

See previous reply.

> isn't this function the same one as the one created before ?  
> If we can't access the one created before, maybe we could the method to a function (i.e. not as a member of an object) at the bottom of the file (or in a util file if we already have such things)

There was a copy in the server portion and a copy in the client portion because I wanted to avoid unnecessarily doing this over the protocol.

Using a shared helper does make more sense so... done.

	Nicolas Chevobbe [:nchevobbe]
	Comment 6 • 8 years ago
mozreview-review-reply

Comment on attachment 8826167 [details]
Bug 1302989 - Make storage inspector work with file:// when # is in the URL

https://reviewboard.mozilla.org/r/104190/#review105088

No problems Mike, I totally understand.
I'll have a look at the new diff shortly, thanks !

	Nicolas Chevobbe [:nchevobbe]
	Comment 7 • 8 years ago
mozreview-review

Comment on attachment 8826167 [details]
Bug 1302989 - Make storage inspector work with file:// when # is in the URL

https://reviewboard.mozilla.org/r/104190/#review105596

Only a couple of minor typos, r+ with those fixed :)

::: devtools/client/storage/test/browser_storage_basic.js:91
(Diff revisions 1 - 2)
>   */
>  function testTree() {
>    let doc = gPanelWindow.document;
> -  for (let item of testCases) {
> -    ok(doc.querySelector("[data-id='" + JSON.stringify(item[0]) + "']"),
> +  for (let [item] of testCases) {
> +    ok(doc.querySelector("[data-id='" + JSON.stringify(item) + "']"),
>         "Tree item " + item[0] + " should be present in the storage tree");

I think `item[0]` should be `item` here

::: devtools/client/storage/test/browser_storage_basic_with_fragment.js:96
(Diff revisions 1 - 2)
>   */
>  function testTree() {
>    let doc = gPanelWindow.document;
> -  for (let item of testCases) {
> -    ok(doc.querySelector("[data-id='" + JSON.stringify(item[0]) + "']"),
> +  for (let [item] of testCases) {
> +    ok(doc.querySelector("[data-id='" + JSON.stringify(item) + "']"),
>         "Tree item " + item[0] + " should be present in the storage tree");

I think `item[0]` should be `item` here

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Assignee
	Comment 8 • 8 years ago
mozreview-review-reply

Comment on attachment 8826167 [details]
Bug 1302989 - Make storage inspector work with file:// when # is in the URL

https://reviewboard.mozilla.org/r/104190/#review105596

> I think `item[0]` should be `item` here

Nope, it should be item[0] although I know that is confusing. It really highlights why these tests should be rewritten but, again, we may as well wait until the conversion to React for that.

> I think `item[0]` should be `item` here

Nope, it should be item[0] although I know that is confusing. It really highlights why these tests should be rewritten but, again, we may as well wait until the conversion to React for that.

	Pulsebot
	Comment 9 • 8 years ago

Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7de18c5dbc39
Make storage inspector work with file:// when # is in the URL r=nchevobbe

	Carsten Book [:Tomcat]
	Comment 10 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7de18c5dbc39