Created attachment 217141 [details] [review]
cr-string: Prevent memory leak

Instead of leaking the existing GString, truncate and append to it
instead.

Created attachment 217142 [details] [review]
cr-om-parser: Stop memory leak

The parser refs the doc handler, so we need to unref it here.

Review of attachment 217141 [details] [review]:

::: src/cr-string.c
@@ +83,3 @@
 	}
 	if (a_string) {
+		g_string_truncate (result->stryng, 0);

Since result->stryng is freshly created, there really is no need to truncate it.

@@ +90,2 @@
 	} else {
 		result->stryng = g_string_new (NULL) ;

Same leak here, too. Since result->strync was created in cr_string_new(), and is empty, you can just delete this branch.

Created attachment 217153 [details] [review]
cr-string: Prevent memory leak

Instead of leaking the existing GString, truncate and append to it
instead.

Created attachment 217154 [details] [review]
cr-om-parser: Stop memory leak

The parser refs the doc handler, so we need to unref it here.



Removed a silly:

    if (status == OK)
         return OK;

    return status;

section.

Comment on attachment 217153 [details] [review]
cr-string: Prevent memory leak

Attachment 217153 [details] pushed as 73b04a6 - cr-string: Prevent memory leak

Review of attachment 217142 [details] [review]:

So in this function first we get the handler:

        status = cr_parser_get_sac_handler (PRIVATE (a_this)->parser,
                                            &sac_handler);

Now assume it existed, so sac_handler != NULL. Later set this pointer as the handler again:

 status = cr_parser_set_sac_handler (PRIVATE (a_this)->parser,
                                            sac_handler);

but cr_parser_set_sac_handler *first* unrefs, then sets, then refs again. That will obviously crash. So cr_parsre_set_sac_handler should be fixed to be safe for setting the current value back. 
(And this suggests that the code in cr-om-parser isn't ever getting a non-NULL handler back...)

::: src/cr-om-parser.c
@@ +154,3 @@
         g_return_val_if_fail (status == CR_OK, status);
 
+        if (!created_handler) {

That looks wrong. It would create sac_handler always, since created_handler is FALSE at this point.

Created attachment 217155 [details] [review]
cr-om-parser: Stop memory leak

The parser refs the doc handler, so we need to unref it here.



Yeah, I noticed that. I was considering just ripping out the part where we get the current handler - it doesn't make much sense to me, as we're stealing some other object and setting a bunch of function pointers on it. But I didn't want to break anything, even if it was already broken by my eyes.

Let's just only set it if we created a new handler.

Comment on attachment 217155 [details] [review]
cr-om-parser: Stop memory leak

Looks correct to me.

Attachment 217155 [details] pushed as 30ca729 - cr-om-parser: Stop memory leak