Issue #1173 - Protect more entry points with try-catch blocks

Issue #1173 - Protect more entry points with try-catch blocks

Add try-catch blocks to the bodies of 'CPluginClass' functions:
'QueryStatus()', 'Exec()', 'NewStatusProc()', 'PaneWindowProc()'

[Eric, 2015-12-22 14:17:48]

This is almost completely an indentation-change modification, since the only
code change is to add the try-catch statement around these function bodies. The
review tool doesn't show the indentation as well as it might, though.

[Oleksandr, 2016-01-04 10:46:14]

https://codereview.adblockplus.org/29332959/diff/29332966/src/plugin/PluginCl...
File src/plugin/PluginClass.cpp (right):

https://codereview.adblockplus.org/29332959/diff/29332966/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1272: return E_FAIL; //
Nit: there was a comment intended here?

https://codereview.adblockplus.org/29332959/diff/29332966/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1359: return E_FAIL;
I think we should call: return DefWindowProc(hWnd, message, wParam, lParam) here
instead. This is not a COM method so returning E_FAIL doesn't seem right.

https://codereview.adblockplus.org/29332959/diff/29332966/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1597: return E_FAIL;
Same here. I think we should do 'return DefWindowProc(hWnd, message, wParam,
lParam);' instead.

[sergei, 2016-01-04 11:27:36]

https://codereview.adblockplus.org/29332959/diff/29332966/src/plugin/PluginCl...
File src/plugin/PluginClass.cpp (right):

https://codereview.adblockplus.org/29332959/diff/29332966/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1359: return E_FAIL;
On 2016/01/04 10:46:13, Oleksandr wrote:
> I think we should call: return DefWindowProc(hWnd, message, wParam, lParam)
here
> instead. This is not a COM method so returning E_FAIL doesn't seem right.

It's better to use CallWindowProc instead of DefWindowProc here because we are
subclassing the window.

[Eric, 2016-01-04 16:44:25]

Patch set 2 changes the error handling for window procedures.

It also adds a comment for a race condition that may or may not be a defect. The
only time the marked statement could be reached is when a window is closing
down, and it's not clear that it ever does so. Added as a comment rather than a
defect report in the tracker.

https://codereview.adblockplus.org/29332959/diff/29332966/src/plugin/PluginCl...
File src/plugin/PluginClass.cpp (right):

https://codereview.adblockplus.org/29332959/diff/29332966/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1272: return E_FAIL; //
On 2016/01/04 10:46:13, Oleksandr wrote:
> Nit: there was a comment intended here?

Yes. Added.

https://codereview.adblockplus.org/29332959/diff/29332966/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1287: return DefWindowProc(hWnd, message, wParam,
lParam);
Race condition here.

https://codereview.adblockplus.org/29332959/diff/29332966/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1359: return E_FAIL;
On 2016/01/04 11:27:36, sergei wrote:
> It's better to use CallWindowProc instead of DefWindowProc here because we are
> subclassing the window.

Sergei's right.

This observation also points out a defect on line 1287.

https://codereview.adblockplus.org/29332959/diff/29332966/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1597: return E_FAIL;
On 2016/01/04 10:46:13, Oleksandr wrote:
> Same here. I think we should do 'return DefWindowProc(hWnd, message, wParam,
> lParam);' instead.

Done.

Replace with a comment. Moved call to 'DefWindowProc' to the end, outside the
'try' statement, since it doesn't throw.

[Oleksandr, 2016-01-05 02:06:38]

https://codereview.adblockplus.org/29332959/diff/29333139/src/plugin/PluginCl...
File src/plugin/PluginClass.cpp (right):

https://codereview.adblockplus.org/29332959/diff/29333139/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1272: return E_FAIL; // Suppress exceptions, should
log
Not really following the comment. Does this mean that logging will be added in
future change sets? Isn't this the time to add logging?

https://codereview.adblockplus.org/29332959/diff/29333139/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1358: // Suppress exception. Fall through to default
handler.
Empty catch blocks always look awry to me. Maybe we can log at least?

[Eric, 2016-01-05 15:59:44]

Patch set 3 is rebase only.

Patch set 4 adds simple logging messages to the new handlers.

https://codereview.adblockplus.org/29332959/diff/29333139/src/plugin/PluginCl...
File src/plugin/PluginClass.cpp (right):

https://codereview.adblockplus.org/29332959/diff/29333139/src/plugin/PluginCl...
src/plugin/PluginClass.cpp:1272: return E_FAIL; // Suppress exceptions, should
log
On 2016/01/05 02:06:38, Oleksandr wrote:
> Not really following the comment. Does this mean that logging will be added in
> future change sets? Isn't this the time to add logging? 

The only logging we have is in Debug builds. I would like Release build logging
here, particularly for 'logic_error'. Since we don't have it, though, there's no
reason to beggar the present. I've added a Debug build log message here.

[Oleksandr, 2016-01-05 21:02:03]

Thanks, LGTM.

[sergei, 2016-01-05 22:34:51]

LGTM.