Issue 6495401389588480: Issue #1173 - add exception handler to CPluginClass::Invoke (Closed)
DescriptionIssue #1173 - add exception handler to CPluginClass::Invoke
The main change here is adding an exception handler to CPluginClass::Invoke. As
usual, most of the textual changes are indentation. Behavior under catch-all
exception is simply to return a failure value.
Change return value of VARIANT_TRUE to S_OK, the documented return value for
Invoke.
Added some comments about issues raised during auditing the code, in particular
about #1163. Fixed some indentation. Removed a bit of dead code.
Patch Set 1 #
Total comments: 15
Patch Set 2 : #
Total comments: 4
MessagesTotal messages: 9
http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:397: // AUDIT: Explicitly releasing a resource when a container becomes empty looks like a job better suited for shared_ptr I think it's better to create an issue then to put this comment into the code. http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:535: */ All the comments in this file that start with #1163 are redundant here, IMHO. I don't any of them adds in any way to readability of the code. http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:637: // Entry point exception handler Self-evident, IMHO http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:779: { Maybe let's log at least something first
http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:397: // AUDIT: Explicitly releasing a resource when a container becomes empty looks like a job better suited for shared_ptr On 2014/08/21 14:49:56, Oleksandr wrote: > I think it's better to create an issue then to put this comment into the code. Well, I don't know what the issue would be yet; I haven't spent enough time analyzing the code to know everything that's involved here. Is it better to have neither comment nor issue on this? http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:535: */ On 2014/08/21 14:49:56, Oleksandr wrote: > All the comments in this file that start with #1163 are redundant here, IMHO. I > don't any of them adds in any way to readability of the code. These comments are intended to exist only while #1163 is being worked on, because it's not an issue that can get solved with a single review. These comments essentially record a work-in-progress. I don't intend them to be permanent. They'll all go as #1163 gets finished. http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:637: // Entry point exception handler On 2014/08/21 14:49:56, Oleksandr wrote: > Self-evident, IMHO I mentioned this in another review, but I'd like to have some way of using 'grep' to find all the entry points. Suggestions welcome. http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:779: { On 2014/08/21 14:49:56, Oleksandr wrote: > Maybe let's log at least something first We could. Got a suggestion? Right now we've got no general-purpose log message for unknown exceptions. There's not a lot to say since we don't have an exception object. Is just the function name sufficient?
http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:397: // AUDIT: Explicitly releasing a resource when a container becomes empty looks like a job better suited for shared_ptr How about TODO: then? On 2014/09/29 19:16:44, Eric wrote: > On 2014/08/21 14:49:56, Oleksandr wrote: > > I think it's better to create an issue then to put this comment into the code. > > Well, I don't know what the issue would be yet; I haven't spent enough time > analyzing the code to know everything that's involved here. Is it better to have > neither comment nor issue on this? > http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:535: */ ok, as long as you'll be able to keep track of them :) On 2014/09/29 19:16:44, Eric wrote: > On 2014/08/21 14:49:56, Oleksandr wrote: > > All the comments in this file that start with #1163 are redundant here, IMHO. > I > > don't any of them adds in any way to readability of the code. > > These comments are intended to exist only while #1163 is being worked on, > because it's not an issue that can get solved with a single review. These > comments essentially record a work-in-progress. > > I don't intend them to be permanent. They'll all go as #1163 gets finished. http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:779: { Yes, I think it would be sufficient. We could in theory also log thread and process IDs, but I don't remember that ever being useful before for me, so we can just skip it. On 2014/09/29 19:16:44, Eric wrote: > On 2014/08/21 14:49:56, Oleksandr wrote: > > Maybe let's log at least something first > > We could. Got a suggestion? Right now we've got no general-purpose log message > for unknown exceptions. There's not a lot to say since we don't have an > exception object. Is just the function name sufficient?
http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:397: // AUDIT: Explicitly releasing a resource when a container becomes empty looks like a job better suited for shared_ptr On 2014/10/02 20:44:16, Oleksandr wrote: > How about TODO: then? OK. http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:535: */ On 2014/10/02 20:44:16, Oleksandr wrote: > ok, as long as you'll be able to keep track of them :) No problem. That's what grep is for. http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:637: // Entry point exception handler Per a suggestion in another review, I'm changing this simply to mark this function as an entry point. http://codereview.adblockplus.org/6495401389588480/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:779: { On 2014/10/02 20:44:16, Oleksandr wrote: > Yes, I think it would be sufficient. OK. I just used DEBUG_GENERAL, lacking anything specifically for this case yet.
LGTM http://codereview.adblockplus.org/6495401389588480/diff/5685265389584384/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6495401389588480/diff/5685265389584384/src/... src/plugin/PluginClass.cpp:882: I understand this was added here inadvertently, since the code was merged already, right?
http://codereview.adblockplus.org/6495401389588480/diff/5685265389584384/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6495401389588480/diff/5685265389584384/src/... src/plugin/PluginClass.cpp:882: On 2014/10/03 08:31:50, Oleksandr wrote: > I understand this was added here inadvertently, since the code was merged > already, right? I don't understand what you're referring to here. What was added? I just checked the public head; this is the same code that's there currently.
http://codereview.adblockplus.org/6495401389588480/diff/5685265389584384/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6495401389588480/diff/5685265389584384/src/... src/plugin/PluginClass.cpp:882: hm. Trying doing a diff for patchset 1 vs a patchset 2 here. There's a bunch of code added here in patchset 2. On 2014/10/03 15:14:13, Eric wrote: > On 2014/10/03 08:31:50, Oleksandr wrote: > > I understand this was added here inadvertently, since the code was merged > > already, right? > > I don't understand what you're referring to here. What was added? > > I just checked the public head; this is the same code that's there currently.
http://codereview.adblockplus.org/6495401389588480/diff/5685265389584384/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/6495401389588480/diff/5685265389584384/src/... src/plugin/PluginClass.cpp:882: Ah. What you're seeing is an artifact of rebasing the code between the time the first patch set was created and that of the second. The changes are from 0892c5dece26064d992adf5f25c8895ed7ca6e3f, which is a change of yours after we forked a development branch a couple of months ago. 
|
