Closed Bug 1260382 Opened 5 years ago Closed 5 years ago

Storage inspector incorrectly tries to parse invalid localStorage JSON values

Categories

(DevTools :: Storage Inspector, defect, P2)

defect

Tracking

(firefox50 verified)

VERIFIED FIXED
Firefox 50
Tracking Status
firefox50 --- verified

People

(Reporter: pbro, Assigned: Fischer, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files)

Attached image Capture.PNG
STR:
- open the mozilla.org site
- open the console
- enter localStorage.setItem('test', '{foo: "bar"}')
  - note that I have omitted the "" around foo, so that the value isn't strictly valid JSON
- open the storage inspector
- switch to the local storage item in the tree-view
- click on the 'test' item

==> The storage inspector seems to believe the value is JSON so it tries to parse it (and displays a "Parsed Value" in the sidebar), but can't parse it to an object correctly and shows:
{foo:" "bar"}"
and the highlighting is weird.

See screenshot.
What happens is this:
1. JSON.parse is attempted and fails with SyntaxError
2. In addition to JSON, the storage inspector tries to parse the value as a key-value list with various delimiters, like "a=b:c=d:e=f" or "a:b,c:d,e:f". Probably useful for cookies. In this case, the value is guessed to be a one-item key-value list with a ":" delimiter:
key = {foo
value = "bar"}

Of course, a sloppy JSON would be a much better guess. If only there was some more forgiving JSON parser... JSON.parse seems to be too strict for this purpose.
Yes, this bug comes back again and again even though it is the side affect of a feature.

We don't just list JSON... we also try to list URLEncoded JSON and things that loosely look like some form of key value pair.

The obvious thing here is that we should be parsing as an Object Literal and not JSON. There is a very simple library we could use for that here:
https://github.com/daepark/JSOL

We need to preserve the copyright notice and need to tweak the code slightly but this would slightly improve our current implementation... in fact, that would make this a good first bug.
Mentor: mratcliffe
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All
Whiteboard: [good-first-bug lang=js]
Whiteboard: [good-first-bug lang=js] → [good first bug][lang=js]
Hey Everyone,

Thanks for the context.

I was wondering if anyone was working on this.  

This would be my first bug fix and would like to take a swing at it.

Thoughts?

Thanks
Hello Scott, thanks for your interest!

The first thing you need to do is to checkout the Firefox sources from Mercurial or Git and build it. Please follow the hacking guide here:

https://wiki.mozilla.org/DevTools/Hacking

Additionally, if you'd prefer working with the Git clone of the Firefox repository on Github instead of setting up Mercurial, then this MDN page has the info on Git setup:

https://developer.mozilla.org/en-US/docs/Tools/Contributing

After you have a build, you can start hacking the code. This bug will involve replacing the JSON.parse call at this place:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/storage/ui.js#578

by some more forgiving JSON parser. Mike mentioned JSOL: https://github.com/daepark/JSOL, and another library that's worth considering is Human JSON: https://hjson.org

The new third-party library would go into the /devtools/client/shared/vendor folder.
(In reply to Jarda Snajdr [:jsnajdr] from comment #4)
> Hello Scott, thanks for your interest!
> 
> The first thing you need to do is to checkout the Firefox sources from
> Mercurial or Git and build it. Please follow the hacking guide here:
> 
> https://wiki.mozilla.org/DevTools/Hacking
> 
> Additionally, if you'd prefer working with the Git clone of the Firefox
> repository on Github instead of setting up Mercurial, then this MDN page has
> the info on Git setup:
> 
> https://developer.mozilla.org/en-US/docs/Tools/Contributing
> 
> After you have a build, you can start hacking the code. This bug will
> involve replacing the JSON.parse call at this place:
> 
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/storage/ui.
> js#578
> 
> by some more forgiving JSON parser. Mike mentioned JSOL:
> https://github.com/daepark/JSOL, and another library that's worth
> considering is Human JSON: https://hjson.org
> 
> The new third-party library would go into the /devtools/client/shared/vendor
> folder.

Perfect, I will get on this today.  Thank you for the guidance
Trying to reproduce bug locally after running ./mach run and when I enter in localStorage.setItem('test', '{foo: "bar"}') in the web console i get an error. 

[Exception... "Component is not available"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]

Anyone know why?

Thanks
What page are you on? Probably "about:newtab" or something similar, right? localStorage is unavailable in certain special contexts.
(In reply to Jarda Snajdr [:jsnajdr] from comment #7)
> What page are you on? Probably "about:newtab" or something similar, right?
> localStorage is unavailable in certain special contexts.

Thanks, ya I was in about:newtab, went to a new site and it worked.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #2)
> Yes, this bug comes back again and again even though it is the side affect
> of a feature.
> 
> We don't just list JSON... we also try to list URLEncoded JSON and things
> that loosely look like some form of key value pair.
> 
> The obvious thing here is that we should be parsing as an Object Literal and
> not JSON. There is a very simple library we could use for that here:
> https://github.com/daepark/JSOL
> 
> We need to preserve the copyright notice and need to tweak the code slightly
> but this would slightly improve our current implementation... in fact, that
> would make this a good first bug.

Hey Michael, I was digging through the source code and reading your comments.  So in theory I would need to replace the meat of '_extractKeyValPairs' with the JSOL.parse functionality?  You said it might need to be tweaked before implementing.  Is there anything that stood out to you from jsol.js?  Also, would I just copy the entire file and include it in vendors(with copyright notice)? OR is there more to the process for including the JSOL.js file into the codebase?

Thanks
Hello Scott, before you can "require" the jsol.js file, you need to convert it into a CommonJS module (in a node.js style). That means removing the boilerplate that creates a window.JSOL object and leave just the JSOL.parse function, exporting it with:

exports.parse = function...

That should be all the tweaks you need.
(In reply to Jarda Snajdr [:jsnajdr] from comment #10)
> Hello Scott, before you can "require" the jsol.js file, you need to convert
> it into a CommonJS module (in a node.js style). That means removing the
> boilerplate that creates a window.JSOL object and leave just the JSOL.parse
> function, exporting it with:
> 
> exports.parse = function...
> 
> That should be all the tweaks you need.

THANK YOU
Assignee: nobody → scott.stern06
@Mike,
Since this bug hasn't been touched for almost 2 month and I happen to work on the ui.js for another bug as well, I would like to take on this bug.

Now I can parse '{foo: "bar"}' with JSOL.parse lib in Comment #2 and make storage display 'foo:"bar"' by manually pasting to include the JSOL lib.

However, when I put the jsol.js under the /devtools/client/shared/vendor folder and edit /devtools/client/shared/vendor/moz.build to include the jsol.js on build. I get this error message: 
```
['UnsortedError: An attempt was made to add an unsorted sequence to a list. The incoming list is unsorted starting at element 0. We expected "immutable.js" but got "jsol.js"\n']
```

How to include the jsol,js to use ?

Thanks
Flags: needinfo?(mratcliffe)
Assignee: scott.stern06 → fliu
@Fischer: The filenames need to be added to /devtools/client/shared/vendor/moz.build in alphabetic order.
Flags: needinfo?(mratcliffe)
Comment on attachment 8770100 [details]
Bug 1260382 - Storage inspector incorrectly tries to parse invalid localStorage JSON values;

https://reviewboard.mozilla.org/r/63676/#review63360

Excellent patch... thanks for working on this, r+.
Attachment #8770100 - Flags: review?(mratcliffe) → review+
We need a try run before we land this. I have triggered one so we just need to wait for the results.
Looks great, marking for checkin.
Keywords: checkin-needed
does not apply cleanly 

Hunk #1 succeeded at 2 with fuzz 1 (offset 0 lines).
patching file devtools/client/storage/ui.js
Hunk #1 FAILED at 2
Hunk #2 FAILED at 546
2 out of 2 hunks FAILED -- saving rejects to file devtools/client/storage/ui.js.rej
patch failed to apply
abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(fliu)
Keywords: checkin-needed
Comment on attachment 8770100 [details]
Bug 1260382 - Storage inspector incorrectly tries to parse invalid localStorage JSON values;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63676/diff/1-2/
@Tomcat,
Rebase the patch to the latest commit.
Please try again, thank you.
Flags: needinfo?(fliu) → needinfo?(cbook)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/08e6660bfd34
Storage inspector incorrectly tries to parse invalid localStorage JSON values. r=miker
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/08e6660bfd34
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Flags: needinfo?(cbook)
 I have reproduced this bug with Nightly 48.0a1 (2016-03-29) on Windows 8 , 64 Bit !

 This bug's fix is verified with latest Beta 50.0b3 

 Build ID   : 20160929120120
 User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0

 [testday-20160930]
Based on comment 24, I will set the corresponding flags.
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.