Memory leak when initializing CommonMark.Document via commonmark string

[fe9lix]

Hi! I've noticed a memory leak when using CommonMark.Document "standalone" as root node, basically the first example that is given in the README file (just initialize a new Document with a commonmark string). It seems that for this initializer the internal managed property would not be set to true, and then prevent the cmark nodes from being freed in deinit. To fix that, I just added an extension that would set the managed property, sth. like this (I'm sure there are better ways...):

public extension Document {
  // Enforce managed flag to be set when the node is used "standalone", otherwise no freeing of cmark nodes will happen on deinit.
  class func root(_ commonmark: String, options: ParsingOptions = []) throws -> Document {
    let document = try Self.init(commonmark, options: options)
    document.managed = true
    return document
  }
}

Can you confirm this or am I missing sth. in terms of the API or intended usage?

[regexident]

Hi @fe9lix, you did indeed stumble upon a memory leak! Which in turn was preventing several dangling pointers from crashing. (The method further more ignored any parsing options passed to it: #20)

Due to the dangling pointers your fix listed above would most likely cause crashes.

PR #22 aims to address and fix both issues.

[fe9lix]

Hi @regexident , thanks for getting back! I've just skimmed your changes and I'm not familiar with the internal structure of the lib, but I can't see how those changes would lead to deallocation of cmark nodes for the following use case:

var doc: Document? = Document("Some markdown...")
doc = nil

I'd expect the entire tree to get deallocated once I nil the root document – unless this is not the intended usage of a Document or I'm missing some options or manual disposal calls? (otherwise in the example above managed would still be false in Node.swift line 34...)

[regexident]

You're probably right. Your snippet should be right, however patching the framework's existing initializer will most certainly lead to crashes.

Either way with PR #22 the leaks (and related memory issues) should be resolved. :)

[fe9lix]

Yup, this is exactly why I didn't want to patch the Document initializers directly but rather used the workaround above that simply sets the managed property so that a Document's deinit method actually runs cmark_node_free(cmark_node) when the root node is used "standalone". The README in this repo exactly shows that use case and even after merging PR #22, the deallocation issue would persist.

So I guess what's eventually needed is a fix that sets the property internally but makes sure that it doesn't impact other uses of the Document node, which may cause the crashes that you mention (for instance, it seems that Fragment also initializes a Document via a String builder...).