Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#3350 closed defect (fixed)

Incorrect use of DISP_E_EXCEPTION in CPluginUserSettings::Invoke()

Reported by: eric@… Assignee:
Priority: Unknown Milestone: Adblock-Plus-for-Internet-Explorer-1.6
Module: Adblock-Plus-for-Internet-Explorer Keywords:
Cc: oleksandr, sergz Blocked By:
Blocking: Platform: Internet Explorer
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29330798/

Description

Environment

Source code defect.

Observed Defect

Two of the calls to the user settings object are "by index". At present, we are returning DISP_E_EXCEPTION for these errors but we are not filling in the exception structure as specified here:

https://msdn.microsoft.com/en-us/library/windows/desktop/ms221479(v=vs.85).aspx

How to Fix

One option is to fill in the exception structure.

Easier, however, would be to simply return DISP_E_BADINDEX. This is not documented on the above page, but it is supported elsewhere by Microsoft. After all, it's defined in the same header file as DISP_E_EXCEPTION is, just a few lines away. But also see, for example, the following SO question, which shows that (1) Excel returns this value and (2) the .NET runtime has support for interpreting it.

https://stackoverflow.com/questions/16851642/c-sharp-excel-interop-exception-from-hresult-disp-e-badindex

Change History (6)

comment:1 follow-up: Changed 4 years ago by eric@…

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 in reply to: ↑ 1 Changed 4 years ago by sergz

  • Cc oleksandr added

Replying to eric@…:

One option is to fill in the exception structure.

I would proceed with this option. To make it easier we can create a helper method to fill EXCEPINFO and return DISP_E_EXCEPTION.

Easier, however, would be to simply return DISP_E_BADINDEX. This is not documented on the above page, but it is supported elsewhere by Microsoft. After all, it's defined in the same header file as DISP_E_EXCEPTION is, just a few lines away. But also see, for example, the following SO question, which shows that (1) Excel returns this value and (2) the .NET runtime has support for interpreting it.
https://stackoverflow.com/questions/16851642/c-sharp-excel-interop-exception-from-hresult-disp-e-badindex

I would suppose that in the link above IDispatch::Invoke is not called because of early binding, and DISP_E_EXCEPTION is return by DISPID_DEFAULT method and the means of interoperability ensures to raise an exception converted either directly from HRESULT or from COM exception.

Of course, on one hand we may return any error HRESULT code but on the other hand I would personally use only return codes mentioned on the documentation page. Most probably it does work in our case but it looks dirty in context of COM automation.

comment:3 Changed 4 years ago by sergz

  • Cc sergz added

comment:4 Changed 4 years ago by eric@…

I set up a live test on this issue; see https://codereview.adblockplus.org/29331713/

The results of the test are clear. When an IDispatch method returns DISP_E_BADINDEX, the JS statement throws an exception, it's prototype has 'name' property "RangeError", and the exception message is "Subscript out of range".

Thus, regardless of that fact that the error code is DISP_E_BADINDEX is not documented as a standard return from 'IDispatch', it is in fact fully supported within the JavaScript engine of IE.

comment:5 Changed 4 years ago by eric@…

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:6 Changed 3 years ago by oleksandr

  • Milestone set to Adblock-Plus-for-Internet-Explorer-Next
Note: See TracTickets for help on using tickets.