Re: [Libreoffice] Solving an introduced runtime dependency on unreleased Qt version (KDE4 vcl backend)

On Tuesday 22 of November 2011, Michael Meeks wrote:
> On Tue, 2011-11-22 at 18:26 +0100, Lubos Lunak wrote:
> >  Since 3.4 at least, when run with KDE4 integration, LO kind of happens
> > to have a runtime dependency on a yet unreleased Qt version, otherwise LO
> > will abort during some reasonably commonplace operations
>
> > That doesn't quite fit with LO, which occassionally will do UI stuff even
> > from other threads.
>
>       Right - but we guarentee that only one thread at a time will do that -
> protected by the solar mutex. So there should never be two threads
> concurrently inside the X libraries or the Qt toolkit.
>
>       Are you suggesting that Qt stores the thread-id of the thread it is
> initialized in, and then compares that all over the shop to check it is
> the same one ? If so, that seems heinously broken - is there a way we
> can turn that off more generally ?, if not - we shouldn't have a problem
> - vcl should -never- enter Qt or X from more than one thread
> concurrently.

 It's not really broken as such, especially historically (it wasn't that long 
ago when threads in UI apps were just an unnecessary complication and even 
just calling XInitThreads() was a bad idea). It's at least not more broken 
than having one huge global mutex >:).

 And the check seems to exist only in QPixmap and LO's Solar Mutex should 
ensure there's no other problem if this check is avoided. So that looks like 
this solves the problem, if people can fix their installed Qt library.

> >  which means that Qt usage from it should be safe, if the way to
> > avoid the QPixmap check is avoided. But there are other UI elements in
> > LO, such as the fpicker, which AFAIK are not triggered using VCL and I
> > don't know how that is supposed to work.
>
>       IIRC the fpicker for KDE is (was) an out-of-process helper
> (program/kdefilepicker), so that should not be an issue.

 Only the KDE3 one, the KDE4 one is handled in-process. But other Qt UI code 
does not seem to have that check and the Solar mutex should again take care 
of this.

> >  What do you suggest to do about it?
> >
> > PS: I've attached a backport of the Qt change to
> > https://bugs.freedesktop.org/show_bug.cgi?id=40298 .
>
>       Reading the patch, we shouldn't need to XInitThreads, IIRC vcl does
> this for us.

 That is Qt-4.8 code and it makes sense that way, Qt code doesn't really care 
that somebody else somewhere randomly called XInitThreads() themselves. And 
calling it twice is no-op anyway.

>       I suspect that what we really want to do is to do a:
>
>       qApp->moveToThread(QThread::currentThread());
>
>       before we start calling Qt methods each time. Hopefully that is
> reasonably efficient (surely it's just a member variable).

 No, it is not and I expect moving QApplication instances between threads 
really wouldn't work. Additionally this is inherently thread-unsafe and as 
such it's allowed only to move objects from the current thread to a different 
one, not this other way around.

-- 
 Lubos Lunak
 l.lunak@suse.cz