OpenBoard-org / OpenBoard

OpenBoard is a cross-platform interactive whiteboard application intended for use in a classroom setting.
https://openboard.ch/
GNU General Public License v3.0
2.34k stars 423 forks source link

[Bug] OpenBoard's user data directory depends on whether QT_QPA_PLATFORM is set to xcb or wayland #1004

Open maehne opened 3 months ago

maehne commented 3 months ago

Describe the bug

While testing OpenBoard in a KDE Plasma Wayland session, I noticed that its user data directory depends on whether QT_QPA_PLATFORM is set to xcb or wayland.

To Reproduce

  1. Log into a KDE Plasma Wayland session and launch a shell
  2. Launch OpenBoard using QT_QPA_PLATFORM=wayland openboard
$ QT_QPA_PLATFORM=wayland openboard
Persisting document; path is "~/.local/share/OpenBoard/document/OpenBoard Document 2024-06-06 15-18-39.461/metadata.rdf"

OpenBoard loads the documents from ~/.local/share/OpenBoard/document/ (notice the camelcase OpenBoard).

  1. Launch OpenBoard using QT_QPA_PLATFORM=xcb openboard
$ QT_QPA_PLATFORM=xcb openboard    
Qt: Session management error: None of the authentication protocols specified are supported
sAppSettings location:  "/etc/openboard/openboard.config"
Persisting document; path is "~/.local/share/openboard/document/OpenBoard Document 2024-06-06 15-20-23.932/metadata.rdf"

OpenBoard loads the documents from ~/.local/share/openboard/document/ (notice the all lowercase OpenBoard).

Expected behavior

OpenBoard should use the same user data directory independent of the setting of QT_QPA_PLATFORM.

Actual behavior

OpenBoard loads documents from ~/.local/share/OpenBoard/document/ when QT_QPA_PLATFORM=wayland and from ~/.local/share/openboard/document/ when QT_QPA_PLATFORM=xcb. In the latter case, I also get the following message at startup:

grafik

If one clicks on "Proceed", OpenBoard closes without any message. The message appears at every startup. The app can be used after clicking on "Cancel". Maybe, @taeyi1 observed something similar in issue #994 when his documents disappeared.

Additional resources

I have noticed that OpenBoard uses three different data directories in ~/.local/share:

$ ls -al ~/.local/share/?pen*
~/.local/share/openboard:
total 36
drwxr-xr-x  8 mht1 mht1 4096 Jun  6 15:18  .
drwxr-xr-x 87 mht1 mht1 4096 Jun  6 14:15  ..
drwxr-xr-x  2 mht1 mht1 4096 Jun  4 08:21  animationUserDirectory
drwxr-xr-x 10 mht1 mht1 4096 Jun  6 15:38  document
drwxr-xr-x  2 mht1 mht1 4096 Jun  4 08:21 'interactive content'
drwxr-xr-x  3 mht1 mht1 4096 Jun  4 08:21  libraryPalette
drwxr-xr-x  2 mht1 mht1 4096 Jun  4 08:22  log
-rw-r--r--  1 mht1 mht1  176 Jun  6 15:18  OpenBoardUser.config
drwxr-xr-x  3 mht1 mht1 4096 Jun  4 08:21  web-cache

~/.local/share/OpenBoard:
total 52
drwxr-xr-x  9 mht1 mht1  4096 Jun  4 22:29  .
drwxr-xr-x 87 mht1 mht1  4096 Jun  6 14:15  ..
drwxr-xr-x  2 mht1 mht1  4096 Sep  9  2022  animationUserDirectory
-rw-r--r--  1 mht1 mht1   599 Okt 19  2023  cookies.ini
drwxr-xr-x 99 mht1 mht1 12288 Jun  6 15:38  document
-rw-r--r--  1 mht1 mht1     0 Sep 29  2023  history
drwxr-xr-x  2 mht1 mht1  4096 Sep  9  2022 'interactive content'
drwxr-xr-x  3 mht1 mht1  4096 Sep  9  2022  libraryPalette
drwxr-xr-x  2 mht1 mht1  4096 Sep 22  2022  log
-rw-r--r--  1 mht1 mht1   780 Jun  4 22:29  OpenBoardUser.config
drwxr-xr-x  4 mht1 mht1  4096 Sep 29  2023  web-cache
drwxr-xr-x  2 mht1 mht1  4096 Okt 19  2023  web-databases

'~/.local/share/Open Education Foundation':
total 12
drwx------  3 mht1 mht1 4096 Dez  8 19:04 .
drwxr-xr-x 87 mht1 mht1 4096 Jun  6 14:15 ..
drwx------  3 mht1 mht1 4096 Mai 31 18:38 OpenBoard
$ ls -al ~/.local/share/Open\ Education\ Foundation/OpenBoard
total 16
drwx------ 3 mht1 mht1 4096 Mai 31 18:38 .
drwx------ 3 mht1 mht1 4096 Dez  8 19:04 ..
-rw------- 1 mht1 mht1 1612 Mai 31 18:38 OpenBoardstaterc
drwx------ 3 mht1 mht1 4096 Dez  8 19:04 QtWebEngine

Shouldn't it be using a single one?

Context

Additional context

letsfindaway commented 3 months ago

The default user data directory is determined by

QStandardPaths::writableLocation(QStandardPaths::AppLocalDataLocation)

So the question is why this function uses an all lowercase appname on Wayland. I don't have an answer for that.

But this is also the reason for the OpenSankore importer widget. Here is the check: https://github.com/OpenBoard-org/OpenBoard/blob/1665e2e570e5e76c75d9e8a4b7e9a3f0c5071736/src/core/UBOpenSankoreImporter.cpp#L42-L43

If the path contains an all lowercase openboard, but the applicationName() is still OpenBoard, then the replace fails and the check returns true.

For the third user data directory ~/.local/share/Open Education Foundation: this is created by QWebEngine automatically using the full application information including organization. This could probably be changed by setting the QWebEngineProfile::persistentStoragePath() of the default profile.

Vekhir commented 3 months ago

@maehne This might be an issue with Qt and X on your system. During my testing, every platform with the wrong application name had some sort of issue. applicationName gets initialised to the executable name. Also note the sAppSettings location: "/etc/openboard/openboard.config". Normally, this doesn't get printed, because the settings are created once during app initilisation, later skipping the branch https://github.com/OpenBoard-org/OpenBoard/blob/1665e2e570e5e76c75d9e8a4b7e9a3f0c5071736/src/core/UBSettings.cpp#L158-L175 It's interesting that you still get a window - but if Qt fails to properly start and we have a partially-initialised object, I don't think we can do much about it.

On my X11 system, choosing QT_QPA_PLATFORM=xcb yields the correct path ($HOME/.local/share/OpenBoard).

Test results

Correct path:

Lowercase path (no window):

Fails, but prints sAppSettings location: "/etc/openboard/openboard.config":

letsfindaway commented 3 months ago

Today, I had a quite similar experience than you. I don't know why, but OpenBoard was trying to read /etc/openboard/openboard.config, even when I'm not on Wayland and have not set QT_QPA_PLATFORM at all. Instead I get the error message

Qt: Session management error: None of the authentication protocols specified are supported

as you reported. And in fact this causes very indirectly this problem.

This error message is generated before the AppName and OrganizationName are set. So the code to determine the user directory is invoked without this information. In this case it uses the application binary name, which is openboard all lowercase. And later it does not execute this code again, when the AppName and OrganizationName are set.

As a workaround I found on the Internet to

unset SESSION_MANAGER`

before launching OpenBoard. And yes, this helps. But I have not found any information why this error occurs and what could be done to avoid it.

It is strange, on Thursday I did not have this problem. Today, two days later, it exists. In between there was an update of some plasma5 packages to 5.27.9, including plasma5-session. May it be related?

Edit: this plasma update has the following changelog in plasma5-session:

- Add patches to fix ksmserver authentication (CVE-2024-36041, boo#1225774):
  * 0001-Authenticate-local-clients.patch
  * 0002-Remove-iceauth-dependency.patch

So yes, it could be related.

letsfindaway commented 3 months ago

I further analyzed this problem on the OpenBoard side and found the following:

In my opinion the static_cast is wrong. It assumes that the object is always fully constructed, but this is not the case here. If we change this to a dynamic_cast, then UBApplication::app() returns nullptr during this phase. This in turn will then skip the if block in ub_message_output(), so that we will not call userDataDirectory() before UBApplication is fully constructed.

maehne commented 3 months ago

@Vekhir and @letsfindaway: Thanks for your feedback and analysis of the bug. I agree that the reported issue might be related to some updated dependency package. I had been using OpenBoard for several months with QT_QPA_PLATFORM=xcb without that issue and then it started a bit more than a week ago. After today's round of updates, things are back to normal:

$ QT_QPA_PLATFORM=xcb openboard
Persisting document; path is "~/.local/share/OpenBoard/document/OpenBoard Document 2024-06-10 09-25-38.595/metadata.rdf"
$ QT_QPA_PLATFORM=wayland openboard
Persisting document; path is "~/.local/share/OpenBoard/document/OpenBoard Document 2024-06-10 09-26-45.936/metadata.rdf"

@letsfindaway: I agree with your analysis why the user data directory was initialized too early to the wrong value. However, I am not sure that the fix you propose is the most appropriate approach. If the ub_message_output() handler relies on a fully constructed UBApplication object, why isn't it registered after the latter has been constructed that is after main.cpp, line 130 or as part of its construction? The documentation of qInstallMessageHandler() seems not to forbid it. Though, the provided example also registers the handler before the construction of the app object.

If moving the installation of the message handler is not an option, your solution in PR #1010 using a dynamic_cast looks good to me. Am I understanding it right that the error message will anyway get printed by the original handler?

Regarding:

For the third user data directory ~/.local/share/Open Education Foundation: this is created by QWebEngine automatically using the full application information including organization. This could probably be changed by setting the QWebEngineProfile::persistentStoragePath() of the default profile.

IMHO, I think it would be good to have a single place where OpenBoard saves all its data -- including the data stored by QWebEngine.

letsfindaway commented 3 months ago

@letsfindaway: I agree with your analysis why the user data directory was initialized too early to the wrong value. However, I am not sure that the fix you propose is the most appropriate approach. If the ub_message_output() handler relies on a fully constructed UBApplication object, why isn't it registered after the latter has been constructed that is after main.cpp, line 130 or as part of its construction? The documentation of qInstallMessageHandler() seems not to forbid it. Though, the provided example also registers the handler before the construction of the app object.

The reason for registering the message handler before constructing the application is, that UBApplication uses qDebug() output in its constructor, of course after setting application and organization name. An example are the loggings in setupTranslators() or while constructing the QSettings.

IMHO, I think it would be good to have a single place where OpenBoard saves all its data -- including the data stored by QWebEngine.

Fully agreed! Probably stuff for a new feature request?

maehne commented 3 months ago

[...]

IMHO, I think it would be good to have a single place where OpenBoard saves all its data -- including the data stored by QWebEngine.

Fully agreed! Probably stuff for a new feature request?

Done in issue #1011

letsfindaway commented 3 months ago

I further analyzed this problem on the OpenBoard side and found the following:

  • With Qt5 (and probably plasma with this patch for CVE-2024-36041) a Qt session management error occurs during the construction of QApplication. Note that with Qt6 the same error occurs, but somewhere later, so it has not the same consequences.

This is a confirmed regression in KDE Plasma handled here: https://bugs.kde.org/show_bug.cgi?id=487912

letsfindaway commented 3 months ago

I further analyzed this problem on the OpenBoard side and found the following:

  • With Qt5 (and probably plasma with this patch for CVE-2024-36041) a Qt session management error occurs during the construction of QApplication. Note that with Qt6 the same error occurs, but somewhere later, so it has not the same consequences.

This is a confirmed regression in KDE Plasma handled here: https://bugs.kde.org/show_bug.cgi?id=487912

For openSUSE, a fix for this KDE Plasma bug reached the update repositories. This means that OpenBoard now behaves as before and uses the correct directories. Can others confirm that for other distros?

maehne commented 3 months ago

As stated above, the issue was also fixed by package updates in Arch Linux. Nevertheless, I think your PR #1010 is still valid and should be merged.