Can a QDomElement without an ownerDocument cause trouble?

Bart_Vandewoestyne

We are struggling with some nasty crashes in our (multithreaded) application, and we think it has to do with the following:

We often create objects of type QDomElement inside some kind of 'creator' functions, like for example the following:

QDomElement createIt()
{
    QDomDocument doc;
    const auto success = doc.setContent(QString("<Foo/>"));
    assert(success);
    return doc.documentElement();
}

After a call to createIt, doc is out of scope and no longer exists, so the returned QDomElement has no owner document anymore. This means that when calling the creator function like

auto element = createIt();

then element will have no ownerDocument. We confirmed this by checking element.ownerDocument().isNull() which returns true.

What we then often do, is pass element to functions of the following form:

void processElement(const QDomElement& elm);
void modifyElement(QDomElement& elm);

Since we are experiencing mysterious crashes (in a multithreaded environment), we are wondering if passing an element without an ownerDocument to functions is considered as bad practice or not. Are we doing something 'dangerous' here, or is what we do just normal and safe?

JonB

@Bart_Vandewoestyne

And just to verify, replacing with QDomDocument* doc = new QDomDocument; is not something you want to do?

Bart_Vandewoestyne

@JonB said in Can a QDomElement without an ownerDocument cause trouble?:

@Bart_Vandewoestyne

And just to verify, replacing with QDomDocument* doc = new QDomDocument; is not something you want to do?

I am always open for suggestions :-) but what exactly do you mean? Are you suggesting that we should create a pointer to a QDomDocument inside createIt (instead of a local stack object) and then return that pointer instead of the QDomElement that we now have?

JonB

@Bart_Vandewoestyne

Yes of course (or similar, there are many variations).

You write:

After a call to createIt, doc is out of scope and no longer exists, so the returned QDomElement has no owner document anymore

That is only because your variable is a QDocument, which is on the stack. If your variable were a QDocument *, set to a new QDocument, the doc would be on the heap. That means it would not "go out of scope"/"be destroyed", and the returned doc.documentElement() would still have a parent/owner document.

In this case you can still have your function return the doc.documentElement() --- no need to change it to return the actual doc. You can always access the QDocument* doc via the owner/document from the QDomElement.

Of course, now your code becomes responsible for disposing the QDocument when you are done using it, else you'll have a memory leak. Which may or may not be easy for you, depending on your code architecture.

You can await someone who knows better than I to see if your original code as it stands is "safe" to use given that the returned node will have no owner document. meanwhile you could start considering my suggestion...

Bart_Vandewoestyne

@JonB said in Can a QDomElement without an ownerDocument cause trouble?:

[...] You can await someone who knows better than I to see if your original code as it stands is "safe" to use given that the returned node will have no owner document. [...]

I just came across this bugreport: https://bugreports.qt.io/browse/QTBUG-8566

This seems to confirm that at least it is very dangerous to have a QDomElement without a QDomDocument (because it went out of scope and got destructed).

Any further insights, guidelines or best practices concerning QDomElements with or without QDomDocument are still very welcome!

JonB

@Bart_Vandewoestyne

Since you say it is not good to have a QDomElement without a QDomDocument, it seems to me that you have little choice but to change the principle of that function to no longer use QDomDocument as local (stack) variable, new it.

Or I guess you could make it a principle that the caller is responsible for creating a QDomDocument, into which the returned QDomElement is inserted.

Or, put createIt() in a class together with all the accessor functions, and make the class responsible for holding the QDomDocument.

It depends on how your code is structured. Also, are there loads of places these "standalone, temporary" are used or few?

The ultimate point is: the scope/lifetime of the QDomDocument must persist across all the calls your code wants to make accessing the QDomElement returned from the utility functions.

Really, it's not your fault that the code is designed this way, but having these "create" functions create their own document to return a node is not a good design. IMHO.