Differential D16457

Avoid crash during session restore
ClosedPublic
Actions

Authored by tobiasdeiminger on Oct 27 2018, 12:00 PM.

Details

Reviewers
aacid
Commits
R223:d1ea28fc7338: Avoid crash during session restore
Summary

Do all access to the passed KConfigGroup really synchronous to KMainWindow::readProperties, then we're safe.

Currently kxmlgui can't guarantee that the passed KConfigGroup is still valid after our call to Shell::openUrl(). This is because inside Shell::openUrl, QDialog::exec may get called. The stacked event loop processes all kinds of asynchronous events, and litterally *anything* can happen. E.g. incoming ICE and DBus messages may be processed. In bug 395765 it happened that XSMP SafeYourself was processed, which calls KConfigGui::setSessionConfig, which leaves the KConfig pointer inside KConfigGroup dangling.

BUGS: 395765

Test Plan
  • get recent Qt5 and KF5
  • manually save desktop session while a document is open in okular
  • modify ~/.config/session/okular_<sessionid> so that Urls points to non existing file
  • manually restore session with okular -session <session_id>, dialog will open and warn about non existent file
  • okular shall not crash after closing that dialog

Diff Detail

Repository
R223 Okular
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Restricted Application added a project: Okular. · View Herald TranscriptOct 27 2018, 12:00 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
tobiasdeiminger requested review of this revision.Oct 27 2018, 12:00 PM
tobiasdeiminger added a comment.Oct 27 2018, 12:06 PM

This is an Okular specific fix. However any application that uses KMainWindow could run into the same problem, so I wonder if there's a way to harden frameworks kxmlgui against that dangling pointer issue?

aacid accepted this revision.Oct 28 2018, 10:22 PM
aacid added a subscriber: aacid.

Good find :)

To make the patch even a bit more awesome we should count the number of times openUrl fails and adapt desiredTab accordingly.

I.e. if desiredTab is 4 but openinng url 1 fails we need to use 3 as desiredTab and not 4.

But that's an already existing bug so if you don't feel like fixing it, it's fine, we cna fix it later :)

This revision is now accepted and ready to land.Oct 28 2018, 10:22 PM
Closed by commit R223:d1ea28fc7338: Avoid crash during session restore (authored by tobiasdeiminger). · Explain WhyOct 29 2018, 8:45 PM
This revision was automatically updated to reflect the committed changes.
tobiasdeiminger added a comment.Oct 29 2018, 8:54 PM
In D16457#350089, @aacid wrote:

we should count the number of times openUrl fails and adapt desiredTab accordingly.

Good point, will do that. Just landed fix 1 anyway, for the case of being too slow to learn about tab management for fix 2 (considering upcoming release).

tobiasdeiminger added a comment.Nov 1 2018, 8:45 PM
In D16457#350089, @aacid wrote:

I.e. if desiredTab is 4 but openinng url 1 fails we need to use 3 as desiredTab and not 4.

@aacid With D16457 applied, Okular already behaves as I would expect.

For example, let's say we've got

ActiveTab=3
Urls[$e]=file:$HOME/file1.pdf,file:$HOME/file2.pdf,$HOME/file3.pdf,$HOME/file4.pdf

where file1.pdf is non-readable and fails to open. ActiveTab=3 means "make the 4th tab active" (because 0-based). And what actually happens is, 4th tab becomes active. The 1st tab for file1.pdf is allocated too, but non-active and empty. Imo this behavior is good. Can you confirm this?

Without D16457 applied and running in my old non-segfault-reproducing-environment, Okular always makes the failed tab active. No matter what ActiveTab is set to. Maybe that's what you had observed?

aacid added a comment.Nov 1 2018, 10:21 PM
In D16457#352447, @tobiasdeiminger wrote:
In D16457#350089, @aacid wrote:

I.e. if desiredTab is 4 but openinng url 1 fails we need to use 3 as desiredTab and not 4.

@aacid With D16457 applied, Okular already behaves as I would expect.

For example, let's say we've got

ActiveTab=3
Urls[$e]=file:$HOME/file1.pdf,file:$HOME/file2.pdf,$HOME/file3.pdf,$HOME/file4.pdf

where file1.pdf is non-readable and fails to open. ActiveTab=3 means "make the 4th tab active" (because 0-based). And what actually happens is, 4th tab becomes active. The 1st tab for file1.pdf is allocated too, but non-active and empty. Imo this behavior is good. Can you confirm this?

Without D16457 applied and running in my old non-segfault-reproducing-environment, Okular always makes the failed tab active. No matter what ActiveTab is set to. Maybe that's what you had observed?

I didn't observe anything because i didn't try it and just read the code. If openUrl produces an empty tab I guess we're fine :)

Revision Contents
Changeset List

PathPackages
Mshell/shell.cpp (2 lines)

Diff 44465

View Options

shell/shell.cpp

Loading...