Windows: Don't display error dialogs
ClosedPublic

Authored by kfunk on Oct 17 2016, 8:06 PM.

Details

Summary

Some Windows API functions like GetDiskFreeSpaceEx may produce error
dialogs. This is not desired. Fix this by temporarily blocking errors by
calling SetThreadErrorMode with certain error suppressing flags.

BUG: 371012
FIXED-IN: 5.28.0

Diff Detail

Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
kfunk updated this revision to Diff 7475.Oct 17 2016, 8:06 PM
kfunk retitled this revision from to Windows: Don't display error dialogs.
kfunk updated this object.
kfunk edited the test plan for this revision. (Show Details)

Can't really comment on this, though.
While I'm a huge fan of RAII you don't seem to be returning early from that function, can't you just set the value and set it back at the end without this new class?

Meanwhile I'm skeptical of the need to "set it back". Is there any case where such error dialog boxes would be desirable? It seems like their existence is an ancient-app-compatibility thing. The documentation for SetErrorMode recommends setting SEM_FAILCRITICALERRORS at app startup and leaving it that way.

brauch edited edge metadata.Oct 17 2016, 8:24 PM

The other interesting question is why we even query for free disk space on all drives, but I guess this patch still makes sense in any case.

kfunk updated this revision to Diff 7476.Oct 17 2016, 8:33 PM
kfunk edited edge metadata.

SetErrorMode -> SetThreadErrorMode (thread-safe)

kfunk added a comment.Oct 17 2016, 8:37 PM

Can't really comment on this, though.

While I'm a huge fan of RAII you don't seem to be returning early from that function

RAII is not only useful for cases where you return early.

..., can't you just set the value and set it back at the end without this new class?

I'm fairly sure, we'll need to guard other WinAPI calls as well. QStorageInfo from qtbase uses SetErrorMode/GetErrorMode a lot

kfunk added a comment.Oct 17 2016, 8:38 PM

Meanwhile I'm skeptical of the need to "set it back". Is there any case where such error dialog boxes would be desirable? It seems like their existence is an ancient-app-compatibility thing. The documentation for SetErrorMode recommends setting SEM_FAILCRITICALERRORS at app startup and leaving it that way.

I'm using a similar approach as Qt does. If some application decides to use a different "global" error mode, we should not interfere with it just because Solid wants to. This is a general purpose library after all.

The user who originally reported the issue confirms that this patch fixes it.

Looks good to me.
But just to be 1000% correct please delete the new operator.

kfunk updated this revision to Diff 7495.Oct 18 2016, 7:17 AM

Add Q_DISABLE_COPY

vonreth accepted this revision.Oct 18 2016, 7:31 AM
vonreth added a reviewer: vonreth.
This revision is now accepted and ready to land.Oct 18 2016, 7:31 AM
brauch requested changes to this revision.Oct 18 2016, 9:04 AM
brauch edited edge metadata.

Since it fixes the issue and looks sensible to me, +1

This revision now requires changes to proceed.Oct 18 2016, 9:04 AM
brauch accepted this revision.Oct 18 2016, 9:04 AM
brauch edited edge metadata.

Er, oops.

This revision is now accepted and ready to land.Oct 18 2016, 9:04 AM
kfunk closed this revision.Oct 18 2016, 10:36 AM

Pushed:

commit f544380db86301f4833b2149dd46dca47acc8042
Author: Kevin Funk <kfunk@kde.org>
Date: Mon Oct 17 22:02:26 2016 +0200

Windows: Don't display error dialogs