1356943 - Base64-encoded, IP addresses and other values are parsed as an array in Dev Tools > Storage Inspector

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect, P5)

Description

[x22RWEN]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170330203839

Steps to reproduce:

Steps to reproduce:
1 - Go to https://mozilla.org
2 - In the dev console, run `document.cookie='test=aGVsbG93b3JsZA==';`
3 - Open the Storage Inspector tab (you may have to enable the tab in the Dev Tools preferences)
4 - Notice that the 'Parsed Value' tree displays the value as an array with 3 elements
--> It'd be nice if the Base64-encoded value were treated as a string instead

Version: Firefox Developer Edition, 54.0a2 (2017-04-16) (64-bit)

Comment 1

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

Here we are splitting the "=" assuming a name, value pair and getting it wrong.

We should fix this and add some extra smarts to our code... one obvious thing is encoding e.g. MD5, Base64 etc.

Comment 2

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

Filter on HOTFROG.

Comment 3

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

Chriso's validator.js is great for this kind of thing and is under the MIT license:
https://github.com/chriso/validator.js

We don't need the whole validator, just the validation functions that we need.

We can at least check for the following and display them as appropriate:
  - Base64
  - dataURI
  - email
  - FQDN
  - IP
  - ISBN
  - ISIN
  - ISSN
  - Mac Address
  - MD5 & SHA1 (SHA1 has 40 chars and MD5 32)
  - Mobile Phone
  - URL
  - UUID
  - Credit card number

We should also include a few changes from pull requests e.g. adding Mobile number formats from Hong Kong.

These are things covered by the validator that we display as an object when they should be displayed as strings.

Objects that do not fit these criteria can still be displayed as a smartly split object.

Comment 4

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

The rest of the validation methods could be useful so I will leave them in the library.

Comment 5

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

Attachment: Example-parsing-errors.png

To see how badly this bug affects the side panel take a look at some images from when the value is set to any of the affected types.

Comment 6

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

The following values can be used for testing:

"[e-mail]",
"www.google.co.uk",
"01:AB:03:04:05:06",
"192.168.1.1",
"xn--froschgrn-x9a.co.uk",
"1.0.4",
"#ff0034",
"9-1",
"10,123",
"true",
"2009-05-19 14:39:22-01",
"data:,Hello World!",

Comment 7

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

Everything else that uses the following separators should be displayed in a tree:
["=", ":", "~", "#", "&", "*", ",", "."]

Comment 8

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

Attachment: Bug 1356943 - Only show a tree in the storage sidebar when it is useful

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

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8876321 [details]
Bug 1356943 - Only show a tree in the storage sidebar when it is useful

https://reviewboard.mozilla.org/r/147732/#review153434

::: devtools/client/storage/ui.js:216
(Diff revision 1)
>  StorageUI.prototype = {
>  
>    storageTypes: null,
>    sidebarToggledOpen: null,
>    shouldLoadMoreItems: true,
> +  sidebarParseTreeHidden: true,

It seems that this new property is only used in tests. In this file it is only set, never read.
I feel a bit uncomfortable with adding test-only code to production code.
Also, checking the value of this property from the test doesn't assert that the tree is indeed visible or not in the sidebar. Couldn't the test check the tree component itself? Does it have a state we can check? Or some DOM objects?

Comment 10

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

Comment on attachment 8876321 [details]
Bug 1356943 - Only show a tree in the storage sidebar when it is useful

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

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8876321 [details]
Bug 1356943 - Only show a tree in the storage sidebar when it is useful

https://reviewboard.mozilla.org/r/147732/#review154972

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9647f9c56e13
Only show a tree in the storage sidebar when it is useful r=pbro

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_storage_dynamic_updates_cookies.js, browser_storage_sidebar_update.js and browser_storage_values.js:

https://hg.mozilla.org/integration/autoland/rev/8b9b0a91608ef9d11400bb1b20ae4dd06f34f22b

Push with failures (later push has them on Linux): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9647f9c56e137e114af6de86a5cf9383611975fc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=108917543&repo=autoland

09:53:41     INFO - TEST-START | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js
09:53:41     INFO - TEST-INFO | started process screenshot
09:53:42     INFO - TEST-INFO | screenshot: exit 0
09:53:42     INFO - Buffered messages logged at 09:53:41
09:53:42     INFO - Entering test bound 
09:53:42     INFO - Adding a new tab with URL: http://test1.example.org/browser/devtools/client/storage/test/storage-updates.html
09:53:42     INFO - Tab added and finished loading
09:53:42     INFO - Found a window: complete
09:53:42     INFO - Opening the storage inspector
09:53:42     INFO - Opening the toolbox
09:53:42     INFO - Console message: [JavaScript Warning: "XUL box for vbox element contained an inline label child, forcing all its children to be wrapped in a block." {file: "chrome://devtools/content/storage/storage.xul" line: 0}]
09:53:42     INFO - Waiting for the stores to update
09:53:42     INFO - Making sure that the toolbox's frame is focused
09:53:42     INFO - TEST-PASS | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | Sidebar is initially hidden - 
09:53:42     INFO - TEST-PASS | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | row found with id "c1{9d414cc5-8319-0a04-0586-c0a6ae01670a}test1.example.org{9d414cc5-8319-0a04-0586-c0a6ae01670a}/browser" - 
09:53:42     INFO - selecting row "c1{9d414cc5-8319-0a04-0586-c0a6ae01670a}test1.example.org{9d414cc5-8319-0a04-0586-c0a6ae01670a}/browser"
09:53:42     INFO - TEST-PASS | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | found property c1 - 
09:53:42     INFO - TEST-PASS | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | rule c1 matched for property c1 - 
09:53:42     INFO - TEST-PASS | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | rule c1.Path matched for property Path - 
09:53:42     INFO - Buffered messages finished
09:53:42     INFO - TEST-UNEXPECTED-FAIL | devtools/client/storage/test/browser_storage_dynamic_updates_cookies.js | rule .c1 did not match any property - 
09:53:42     INFO - Stack trace:
09:53:42     INFO - chrome://mochitests/content/browser/devtools/client/storage/test/head.js:onAllRulesMatched:450
09:53:42     INFO - resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:process:922
09:53:42     INFO - resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:walkerLoop:806
09:53:42     INFO - Not taking screenshot here: see the one that was previously logged

Comment 14

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

Attachment: Bug 1356943 - Only show a tree in the storage sidebar when it is useful r?pbro

Comment 15

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

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e17a2d88817
Only show a tree in the storage sidebar when it is useful r=pbro

Comment 17

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/6e17a2d88817