Issues #1173 - Add exception handler to entry point FilterLoader

Issues #1173 - Add exception handler to entry point FilterLoader

FilterLoader() in PluginTabBase.cpp is a thread-main function and thus an
entry point. Add an exception handler around the body of that function.

The symptom in #1654 arises from a race condition between loading filters and
terminating the application. This change set does not fix this problem
properly, which would require thinking through graceful termination of the
plugin as a whole. The exception thrown is a SEH (system) exception, not a
C++ exception. We are not compiling with /EHa, which causes 'catch(...)'
clauses to catch SEH exceptions. Thus the present code isn't even a stop-gap
for #1654.

[Eric, 2015-02-12 19:40:58]

I would prefer to get this committed quickly and change it later rather than
using this as an occasion to yet-again debate some kind of uniform policy around
entry point handling.

And then there's the old adage: "Never let the best be the enemy of the good."

[sergei, 2015-02-12 23:08:47]

I would say it's not related to #1654, it's only #1173.

> The symptom in #1654 arises from a race condition between loading filters and
terminating the application.
> This change set does not fix this problem properly,...
It does not fix the problem even partially because it does not address anything
related to the race condition.
> As a stop-gap, this change merely truncates the exception caused by the race
condition.
Which exception? I guess, there is no any c++ exception caused by this race
condition.
> The benefit from this stop-gap is that it eliminates the system dialog box
caused by the crash.
But it does not eliminate it.

[Oleksandr, 2015-02-13 03:47:01]

I guess the real problem in LoadHideFilters is not that something is throwing an
unhandled exception, but the segmentation fault. As in access violation happens.
I don't think we should be aiming at ever catching those - that'll be just bad
practice. Even though we could do something like this for debug builds:
http://stackoverflow.com/a/918891. In any case I don't really mind having this
try-catch block, but I am not yet sure it will serve any purpose.


On 2015/02/12 23:08:47, sergei wrote:
> I would say it's not related to #1654, it's only #1173.
> 
> > The symptom in #1654 arises from a race condition between loading filters
and
> terminating the application.
> > This change set does not fix this problem properly,...
> It does not fix the problem even partially because it does not address
anything
> related to the race condition.
> > As a stop-gap, this change merely truncates the exception caused by the race
> condition.
> Which exception? I guess, there is no any c++ exception caused by this race
> condition.
> > The benefit from this stop-gap is that it eliminates the system dialog box
> caused by the crash.
> But it does not eliminate it.

[sergei, 2015-02-13 10:01:40]

JIC, I want to add that I also don't mind about it but the issue number should
be 1173.

FYI:
https://msdn.microsoft.com/en-us/library/aa753617(v=vs.85).aspx#wsbe_Exceptio...

[Eric, 2015-02-13 14:23:54]

I changed both the issue title and its description. Please read the new
description.

After digging into this a bit more, it's apparent that the present change
doesn't actually catch the error in #1654 because of differences in exception
handling.

[sergei, 2015-02-13 14:41:40]

> We are not compiling with /EHa, which causes 'catch(...)'
And we should not do it because it's safer to crash instead of hiding of the
corrupted state.

http://codereview.adblockplus.org/5351131051982848/diff/5629499534213120/src/...
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/5351131051982848/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:98: // In this particular case, it's OK to simply
suppress the exception.
This comment is not relevant as well.

[Eric, 2015-02-13 15:13:52]

http://codereview.adblockplus.org/5351131051982848/diff/5629499534213120/src/...
File src/plugin/PluginTabBase.cpp (right):

http://codereview.adblockplus.org/5351131051982848/diff/5629499534213120/src/...
src/plugin/PluginTabBase.cpp:98: // In this particular case, it's OK to simply
suppress the exception.
On 2015/02/13 14:41:40, sergei wrote:
> This comment is not relevant as well.
This wording is indeed not right. I've changed it to something generic.

[sergei, 2015-02-13 15:15:26]

LGTM without the paragraph "The symptom in #1654 arises from a race condition
between loading filters ..." in the commit message.

[Oleksandr, 2015-02-17 12:21:23]

LGTM