Bugfix: free incomplete containers when JSON parsing fails

related to #2755

diff --git a/include/con.h b/include/con.h
index 1c7bb9322a3fa995ab7a0722ace0a3a3fc2e595a..6a7e31bc44331c2d8ac9f7e305ab42187d49a40a 100644 (file)
--- a/include/con.h
+++ b/include/con.h
@@ -25,6 +25,12 @@ Con *con_new_skeleton(Con *parent, i3Window *window);
  */
 Con *con_new(Con *parent, i3Window *window);
 
+/**
+ * Frees the specified container.
+ *
+ */
+void con_free(Con *con);
+
 /**
  * Sets input focus to the given container. Will be updated in X11 in the next
  * run of x_push_changes().

diff --git a/src/con.c b/src/con.c
index 6bbe692fd59cfd92e5e146fab9e4c2df3152d039..9797afa658a2b9cdc2cdd8e38b2f7958ac7088d1 100644 (file)
--- a/src/con.c
+++ b/src/con.c
@@ -73,6 +73,30 @@ Con *con_new(Con *parent, i3Window *window) {
     return new;
 }
 
+/*
+ * Frees the specified container.
+ *
+ */
+void con_free(Con *con) {
+    free(con->name);
+    FREE(con->deco_render_params);
+    TAILQ_REMOVE(&all_cons, con, all_cons);
+    while (!TAILQ_EMPTY(&(con->swallow_head))) {
+        Match *match = TAILQ_FIRST(&(con->swallow_head));
+        TAILQ_REMOVE(&(con->swallow_head), match, matches);
+        match_free(match);
+        free(match);
+    }
+    while (!TAILQ_EMPTY(&(con->marks_head))) {
+        mark_t *mark = TAILQ_FIRST(&(con->marks_head));
+        TAILQ_REMOVE(&(con->marks_head), mark, marks);
+        FREE(mark->name);
+        FREE(mark);
+    }
+    free(con);
+    DLOG("con %p freed\n", con);
+}
+
 static void _con_attach(Con *con, Con *parent, Con *previous, bool ignore_focus) {
     con->parent = parent;
     Con *loop;

diff --git a/src/load_layout.c b/src/load_layout.c
index 7961e17f96a2ef75e420014d99802634c75e995d..0fa3e85bc5269ac1cea4e95431598e97a22cb516 100644 (file)
--- a/src/load_layout.c
+++ b/src/load_layout.c
@@ -18,6 +18,7 @@
 /* TODO: refactor the whole parsing thing */
 
 static char *last_key;
+static int incomplete;
 static Con *json_node;
 static Con *to_focus;
 static bool parsing_swallows;
@@ -68,6 +69,9 @@ static int json_start_map(void *ctx) {
                 json_node->name = NULL;
                 json_node->parent = parent;
             }
+           /* json_node is incomplete and should be removed if parsing fails */
+           incomplete++;
+           DLOG("incomplete = %d\n", incomplete);
         }
     }
     return 1;
@@ -166,6 +170,8 @@ static int json_end_map(void *ctx) {
         LOG("Creating window\n");
         x_con_init(json_node);
         json_node = json_node->parent;
+       incomplete--;
+       DLOG("incomplete = %d\n", incomplete);
     }
 
     if (parsing_swallows && swallow_is_empty) {
@@ -634,6 +640,7 @@ void tree_append_json(Con *con, const char *filename, char **errormsg) {
     yajl_status stat;
     json_node = con;
     to_focus = NULL;
+    incomplete = 0;
     parsing_swallows = false;
     parsing_rect = false;
     parsing_deco_rect = false;
@@ -649,6 +656,12 @@ void tree_append_json(Con *con, const char *filename, char **errormsg) {
         if (errormsg != NULL)
             *errormsg = sstrdup((const char *)str);
         yajl_free_error(hand, str);
+       while (incomplete-- > 0) {
+         Con *parent = json_node->parent;
+         DLOG("freeing incomplete container %p\n", json_node);
+         con_free(json_node);
+         json_node = parent;
+       }
     }
 
     /* In case not all containers were restored, we need to fix the

diff --git a/src/tree.c b/src/tree.c
index 82a4756ca2fb9d95543c12537ee941a83e2e12ce..296a6a3785da48b281409f0792a2ce6bad51ef05 100644 (file)
--- a/src/tree.c
+++ b/src/tree.c
@@ -320,22 +320,7 @@ bool tree_close_internal(Con *con, kill_window_t kill_window, bool dont_kill_par
         DLOG("parent container killed\n");
     }
 
-    free(con->name);
-    FREE(con->deco_render_params);
-    TAILQ_REMOVE(&all_cons, con, all_cons);
-    while (!TAILQ_EMPTY(&(con->swallow_head))) {
-        Match *match = TAILQ_FIRST(&(con->swallow_head));
-        TAILQ_REMOVE(&(con->swallow_head), match, matches);
-        match_free(match);
-        free(match);
-    }
-    while (!TAILQ_EMPTY(&(con->marks_head))) {
-        mark_t *mark = TAILQ_FIRST(&(con->marks_head));
-        TAILQ_REMOVE(&(con->marks_head), mark, marks);
-        FREE(mark->name);
-        FREE(mark);
-    }
-    free(con);
+    con_free(con);
 
     /* in the case of floating windows, we already focused another container
      * when closing the parent, so we can exit now. */

diff --git a/testcases/t/215-layout-restore-crash.t b/testcases/t/215-layout-restore-crash.t
index 4430dac8f7b3fc173a5a822dcde5cbdce3b3eba5..5b34c29c751d708e43b345de13976275222cc4b5 100644 (file)
--- a/testcases/t/215-layout-restore-crash.t
+++ b/testcases/t/215-layout-restore-crash.t
@@ -131,14 +131,103 @@ print $fh <<'EOT';
 EOT
 $fh->flush;
 my $reply = cmd "append_layout $filename";
-diag('reply = ' . Dumper($reply));
+ok(!$reply->[0]->{success}, 'IPC reply did not indicate success');
 
 does_i3_live;
 
-ok(!$reply->[0]->{success}, 'IPC reply did not indicate success');
 
 close($fh);
 
+################################################################################
+# another file with a superfluous trailing comma (issue #2755)
+################################################################################
+
+subtest 'issue 2755' => sub {
+    plan tests => 4;
+    $ws = fresh_workspace;
+
+    @content = @{get_ws_content($ws)};
+    is(@content, 0, 'no nodes on the new workspace yet');
+
+    ($fh, $filename) = tempfile(UNLINK => 1);
+    print $fh <<'EOT';
+// vim:ts=4:sw=4:et
+{
+    // splith split container with 2 children
+    "border": "normal",
+    "floating": "auto_off",
+    "layout": "splith",
+    "percent": null,
+    "type": "con",
+    "nodes": [
+        {
+            "border": "normal",
+            "current_border_width": 2,
+            "floating": "auto_off",
+            "geometry": {
+               "height": 860,
+               "width": 1396,
+               "x": 1922,
+               "y": 38
+            },
+            "name": "Chromium1",
+            "percent": 0.5,
+            "swallows": [
+               {
+                  "class": "^Chromium$",
+               // "instance": "^chromium$",
+               // "title": "^Git\\ Tutorial\\ \\-\\ corp\\ \\-\\ Chromium$",
+               // "transient_for": "^$",
+               // "window_role": "^browser$"
+               }
+            ],
+            "type": "con"
+        },
+        {
+            "border": "normal",
+            "current_border_width": 2,
+            "floating": "auto_off",
+            "geometry": {
+               "height": 1040,
+               "width": 956,
+               "x": 2,
+               "y": 38
+            },
+            "name": "Chromium2",
+            "percent": 0.5,
+            "swallows": [
+               {
+                  "class": "^Chromium$",
+               // "instance": "^chromium$",
+               // "title": "^Nutanix\\ \\-\\ Prod\\ \\-\\ Sign\\ In\\ \\-\\ Chromium$",
+               // "transient_for": "^$",
+               // "window_role": "^browser$"
+               }
+            ],
+            "type": "con"
+        }
+    ]
+}
+
+EOT
+    $fh->flush;
+    $reply = cmd "append_layout $filename";
+    ok(!$reply->[0]->{success}, 'IPC reply indicated success');
+
+    does_i3_live;
+
+    # Move to a different workspace rendered the half-attached con’s con->parent
+    # invalid.
+    fresh_workspace;
+
+    cmd '[urgent=latest] focus';
+    $reply = cmd 'scratchpad show';
+
+    does_i3_live;
+
+    close($fh);
+};
+
 ################################################################################
 # wrong percent key in a child node
 ################################################################################