Closed Bug 1454103 Opened 2 years ago Closed 2 years ago

Correct display of local and sessionStorage in DevTools

Categories

(DevTools :: General, enhancement, P2)

57 Branch
enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: miker, Assigned: miker)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

The web console and debugger both make use of reps, which doesn't take account that local and sessionStorage are special objects.

Adding a "property" to localStorage doesn't add a property but does add a new entry to the storage type. This means that we shouldn't show any properties except for length.

Also, we currently fail to display keys that have the same name as anything on the prototype.

We also neglect to show an <entries> node even though storage types are collections of name, value entries.
The attached images show the web console and debugger after this fix.
tried it and works like a charm :) 
it will be even better when we'll autoexpand <entries> node in the object inspector.

Would you mind creating a PR in devtools-core instead of modifying bundles here ? I'm planning a new reps bundle release this week so we won't have to wait long.

Also, can we add a mochitest to make sure we can inspect localStorage and sessionStorage as expected in the console ?
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #6)
> tried it and works like a charm :) 
> it will be even better when we'll autoexpand <entries> node in the object
> inspector.
> 
> Would you mind creating a PR in devtools-core instead of modifying bundles
> here ? I'm planning a new reps bundle release this week so we won't have to
> wait long.
> 

Okay, so why not do both? I have created a pull request at https://github.com/devtools-html/devtools-core/pull/1046 which updates devtools-connection and devtools-reps... it is a harmless patch that will do nothing until the object inspector sends the new type.

> Also, can we add a mochitest to make sure we can inspect localStorage and
> sessionStorage as expected in the console ?

Test coming soon.
By "why not do both?" I mean why not keep the changes in to devtools-reps and devtools-connection in this patch? I just means we get the changes as soon as this patch lands.
Thanks for the PR on Github Mike, I'll review it shortly.

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #8)
> By "why not do both?" I mean why not keep the changes in to devtools-reps
> and devtools-connection in this patch? I just means we get the changes as
> soon as this patch lands.

That's a solution indeed, but I fear the changes gets overridden in the next reps/debugger release.
For reps, that's fine, we can wait for the PR to land and include it in the next bundle.

For debugger, it's a bit more tricky since they do multiple release per week, and we need to push
a new version of devtools-reps to npm so it can be added as a dependency in the debugger.
I think modifying the debugger bundle will add complexity the release process for the debugger team.

So maybe here we could: keep the change to reps.js, add a mochitest for console only.
And then, when I update the debugger to use the new reps version, I'll add a test for the debugger as well.

How does that sound to you Mike ?
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)
> Thanks for the PR on Github Mike, I'll review it shortly.
> 
> (In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from
> comment #8)
> > By "why not do both?" I mean why not keep the changes in to devtools-reps
> > and devtools-connection in this patch? I just means we get the changes as
> > soon as this patch lands.
> 
> That's a solution indeed, but I fear the changes gets overridden in the next
> reps/debugger release.
> For reps, that's fine, we can wait for the PR to land and include it in the
> next bundle.
> 
> For debugger, it's a bit more tricky since they do multiple release per
> week, and we need to push
> a new version of devtools-reps to npm so it can be added as a dependency in
> the debugger.
> I think modifying the debugger bundle will add complexity the release
> process for the debugger team.
> 
> So maybe here we could: keep the change to reps.js, add a mochitest for
> console only.
> And then, when I update the debugger to use the new reps version, I'll add a
> test for the debugger as well.
> 
> How does that sound to you Mike ?

Sounds like a plan.
Comment on attachment 8967906 [details]
Bug 1454103 - Correct display of local and sessionStorage in DevTools

https://reviewboard.mozilla.org/r/236600/#review243682

Looks good to me, thanks Mike.
I only have a comment in the test where there could be a race condition.
For the server side test, we can file a follow up so this doesn't block this patch (which I think is fien since we do have the client-side test).

::: devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_local_session_storage.js:14
(Diff revision 2)
> +const TEST_URI = "http://example.com/browser/devtools/client/webconsole/" +
> +                 "test/mochitest/test-local-session-storage.html";
> +
> +add_task(async function() {
> +  const hud = await openNewTabAndConsole(TEST_URI);
> +  const nodes = await waitFor(() => findMessages(hud, "Storage"));

there could be a race here, where the first log is displayed but not the second one yet. Maybe we could do `console.log(localStorage, sessionStorage)` in test-local-session-storage.html so we don't have any risks of such thing

::: devtools/client/webconsole/test/mochitest/browser_webconsole_object_inspector_local_session_storage.js:50
(Diff revision 2)
> +
> +  entriesNode.querySelector(".arrow").click();
> +  await onMapOiMutation;
> +
> +  nodes = oi.querySelectorAll(".node");
> +  // There are now 24 nodes, the 4 original ones, and the 20 entries.

nit: erroneous comment (there's 6 nodes :) )

::: devtools/server/actors/object.js:566
(Diff revision 2)
> +      if (isStorage(this.obj)) {
> +        if (name === "length") {
> +          return undefined;
> +        }
> +        return {
> +          configurable: true,
> +          writable: true,
> +          enumerable: true,
> +          value: name
> +        };
> +      }

could we pull this out of the if and keep it as it was before ?

::: devtools/server/actors/object/property-iterator.js:139
(Diff revision 2)
>  }
>  
>  function enumObjectProperties(objectActor, options) {
>    let names = [];
>    try {
> +    if (!ObjectUtils.isStorage(objectActor.obj)) {

can this happen since we have enumStorageEntries ?

::: devtools/server/actors/object/property-iterator.js:291
(Diff revision 2)
> +function enumStorageEntries(objectActor) {
> +  // Iterating over local / sessionStorage entries goes through various
> +  // intermediate objects - an Iterator object, then a 2-element Array object,
> +  // then the actual values we care about. We don't have Xrays to Iterator
> +  // objects, so we get Opaque wrappers for them.
> +  let raw = objectActor.obj.unsafeDereference();
> +  let keys = [];
> +  for (let i = 0; i < raw.length; i++) {
> +    keys.push(raw.key(i));
> +  }
> +  return {
> +    [Symbol.iterator]: function* () {
> +      for (let key of keys) {
> +        let value = raw.getItem(key);
> +        yield [ key, value ].map(val => gripFromEntry(objectActor, val));
> +      }
> +    },
> +    size: keys.length,
> +    propertyName(index) {
> +      return index;
> +    },
> +    propertyDescription(index) {
> +      let key = keys[index];
> +      let val = raw.getItem(key);
> +      return {
> +        enumerable: true,
> +        value: {
> +          type: "storageEntry",
> +          preview: {
> +            key: gripFromEntry(objectActor, key),
> +            value: gripFromEntry(objectActor, val)
> +          }
> +        }
> +      };
> +    }
> +  };
> +}

this looks good. Could we have a unit test for this, with longString keys and longString values (I assume that's the only reason we create a grip for key and value, since there should always be strings) ?
Attachment #8967906 - Flags: review?(nchevobbe) → review+
Comment on attachment 8967906 [details]
Bug 1454103 - Correct display of local and sessionStorage in DevTools

https://reviewboard.mozilla.org/r/236600/#review246100

::: devtools/server/actors/object.js:566
(Diff revision 2)
> +      if (isStorage(this.obj)) {
> +        if (name === "length") {
> +          return undefined;
> +        }
> +        return {
> +          configurable: true,
> +          writable: true,
> +          enumerable: true,
> +          value: name
> +        };
> +      }

Done
Comment on attachment 8967906 [details]
Bug 1454103 - Correct display of local and sessionStorage in DevTools

https://reviewboard.mozilla.org/r/236600/#review243682

> there could be a race here, where the first log is displayed but not the second one yet. Maybe we could do `console.log(localStorage, sessionStorage)` in test-local-session-storage.html so we don't have any risks of such thing

I decided to log the messages directly from the test and wait for each message individually because that gives us finer grained control. I think this makes more sense from a webconsole test perspective anyhow.

> could we pull this out of the if and keep it as it was before ?

Done

> can this happen since we have enumStorageEntries ?

Nope, removed.

> this looks good. Could we have a unit test for this, with longString keys and longString values (I assume that's the only reason we create a grip for key and value, since there should always be strings) ?

There are not unit tests for `enumMapEntries, enumWeakMapEntries, enumSetEntries` or `enumWeakSetEntries` so poor old `enumStorageEntries` is feeling a bit singled out... would you like me to log a bug to create tests for all of them?
Comment on attachment 8967906 [details]
Bug 1454103 - Correct display of local and sessionStorage in DevTools

https://reviewboard.mozilla.org/r/236600/#review243682

> There are not unit tests for `enumMapEntries, enumWeakMapEntries, enumSetEntries` or `enumWeakSetEntries` so poor old `enumStorageEntries` is feeling a bit singled out... would you like me to log a bug to create tests for all of them?

would be great
Bug 1457445 is the property enumerator test bug.
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b860b811cbb0
Correct display of local and sessionStorage in DevTools r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/b860b811cbb0
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
No longer blocks: 1498987
Depends on: 1498987
You need to log in before you can comment on or make changes to this bug.