After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 723428 - Incorrect state when parsing Pocket json file
Incorrect state when parsing Pocket json file
Status: RESOLVED FIXED
Product: json-glib
Classification: Core
Component: Core
git master
Other All
: Normal normal
: ---
Assigned To: json-glib-maint
json-glib-maint
Depends on:
Blocks:
 
 
Reported: 2014-02-01 19:55 UTC by Bastien Nocera
Modified: 2014-02-03 14:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
testcase.c (1.98 KB, text/plain)
2014-02-01 19:56 UTC, Bastien Nocera
  Details
testcase.json (3.51 KB, text/plain)
2014-02-01 19:56 UTC, Bastien Nocera
  Details
tests: Add new test for reader level bug (2.42 KB, patch)
2014-02-01 22:32 UTC, Bastien Nocera
committed Details | Review
reader: When a read() fails, don't track back on end() (1.75 KB, patch)
2014-02-01 22:32 UTC, Bastien Nocera
needs-work Details | Review
reader: When a read() fails, don't track back on end() (1.77 KB, patch)
2014-02-03 14:08 UTC, Bastien Nocera
committed Details | Review

Description Bastien Nocera 2014-02-01 19:55:41 UTC
Attached is the json file and the test program.

$ ./testcase 
** Message: id: 248702728
** Message: url: https://www.youtube.com/watch?v=vNrrpnPlBrk#t=800s
** Message: id: 181195771
** Message: Error getting resolved_url: The member 'resolved_url' is not defined in the object at the current position.
** Message: Error getting given_url: The member 'given_url' is not defined in the object at the current position.
**
ERROR:testcase.c:26:parse_item: code should not be reached
Aborted (core dumped)
Comment 1 Bastien Nocera 2014-02-01 19:56:13 UTC
Created attachment 267794 [details]
testcase.c
Comment 2 Bastien Nocera 2014-02-01 19:56:39 UTC
Created attachment 267795 [details]
testcase.json
Comment 3 Bastien Nocera 2014-02-01 19:58:37 UTC
I can reproduce the problem using json-glib master.
Comment 4 Bastien Nocera 2014-02-01 22:32:16 UTC
Created attachment 267804 [details] [review]
tests: Add new test for reader level bug
Comment 5 Bastien Nocera 2014-02-01 22:32:36 UTC
Created attachment 267805 [details] [review]
reader: When a read() fails, don't track back on end()

When a call to json_reader_read_element() fails if the element
doesn't exist, we need to call json_reader_end_element() to clear
out any errors.

But the _end_element() call will backtrack to the parent node,
when the _read_element() call did not set the child node.

To fix this, leave early from _end_*() calls when an error has
been set.
Comment 6 Emmanuele Bassi (:ebassi) 2014-02-03 14:02:10 UTC
Review of attachment 267805 [details] [review]:

looks reasonable, except for a coding style issue.

it would be great to have a test unit in the JsonReader test suite that checks that read_element() fails without breaking the object state.

also, it would be good to check if read_member() also fails in the same way.

::: json-glib/json-reader.c
@@ +228,3 @@
 json_reader_unset_error (JsonReader *reader)
 {
+  if (reader->priv->error != NULL) {

coding style: the braces should go on a separate line.
Comment 7 Emmanuele Bassi (:ebassi) 2014-02-03 14:03:04 UTC
Review of attachment 267804 [details] [review]:

looks good, disregard the previous "needs a test unit" comment.
Comment 8 Bastien Nocera 2014-02-03 14:08:29 UTC
Created attachment 267954 [details] [review]
reader: When a read() fails, don't track back on end()

When a call to json_reader_read_element() fails if the element
doesn't exist, we need to call json_reader_end_element() to clear
out any errors.

But the _end_element() call will backtrack to the parent node,
when the _read_element() call did not set the child node.

To fix this, leave early from _end_*() calls when an error has
been set.
Comment 9 Bastien Nocera 2014-02-03 14:12:08 UTC
Attachment 267804 [details] pushed as 40abd7a - tests: Add new test for reader level bug
Attachment 267954 [details] pushed as 501c9fb - reader: When a read() fails, don't track back on end()