Error handling in the InDesign SDK

Report · Sep 07, 2010

I've almost reached the first milestone on my InDesign project and before I move on I decided to do a code review. One thing that caught my attention are the InDesign smart pointers and how they are used in the SDK samples and some of the open source stuff.

For example, here is some code from AppearancePlaceBehaviourUI.cpp in function GetCursor().

InterfacePtr<IHierarchy> sourceItem(const_cast<AppearancePlaceBehaviorUI*>(this), UseDefaultIID());
InterfacePtr<IHierarchy> parentItem(sourceItem->QueryParent());
InterfacePtr<IPlaceBehavior> parentBehavior(parentItem, UseDefaultIID());

if (parentBehavior == nil)
{
InterfacePtr<IHierarchy> sourceContent(sourceItem->GetChildCount() ? sourceItem->QueryChild(0) : nil);

    if (sourceContent)
    {
        InterfacePtr<IPlaceBehaviorUI> sourceBehaviorUI(sourceContent, UseDefaultIID());
        cursor = sourceBehaviorUI->GetCursor(globalLocation, modifiers);
    }

Although the code works under normal circumstances, it strikes me as unsafe code.

sourceItem->QueryParent() is called without first checking that sourceItem is not a NULL pointer.
Functions of the sourceItem object are used again (GetChildCount() and QueryChild()).
The pointer sourceContent is checked before passing it to the constructor of sourceContent.
Function GetCursor() is called without checking if sourceBehaviourUI is not NULL.
For code tidiness pointers should really be tested against nil (i.e. if (ptrSomeObject != nil))

It strikes me as being inconsistent and easy to break - unless that is some of these interfaces are guaranteed to return pointers, in which case is there documentation to state as much? What would happen in the case of an exception such as bad_alloc - are they guaranteed not to throw?

I know that in some places (but not all), the samples use an "exception-style" approach of placing code blocks within a "do while(kFalse)" statement. They check for a NULL pointer and if they find one, break out of the "loop". This approach avoids deep nesting code.

It would be great if Adobe gave a statement stating what their code base will do and won't do (i.e. exception safety). A few definative error handling examples for developers wouldn't go amiss either.

Report · Sep 07, 2010

1) yup.

Understandable, though, as IAppearancePlaceBehaviorUI is only referenced in one boss in the same plugin, and as the same boss also has a private implementation kAppearanceHierarchyNodeImpl for IHierarchy, someone might have taken shortcuts here.

2) would be never reached if 1) already crashes.

3) Probably (I did not check inheritance of AppearancePlaceBehaviorUI ) the whole thing just walks back to sourceItem (unless *this is a second child of its parent hierarchy) and could be optimized away. Ah, it is also a weird way to say const_cast ! Probably a mindless copy-paste job from somewhere else, then incompletely edited.

4) yup, see 3).

5) yup.

My bottom line: I still prefer to see more real world source code that explains a concept, rather than have everything hidden in binary until it is polished to highest standards. Do you know the bug report page? Sometimes suggestions make it thru.

Regarding true exceptions, these have been a big no-go until including CS4. I think CS5 finally enables them by default for plugin projects, but I would not throw across stackframes of foreign / InDesign's own code, or even more general Adobe frameworks.

OT: Given your subject line I had hoped for a discussion on the real error handling via global ErrorCode and alike, but unfortunately such a discussion would anyway be very short. We had it during this year's summit - you're not supposed to set the global errorcode unless you're a command. Especially not if you're an observer, most code that executes commands is not prepared for arbitrary failure. Try it yourself to see neat "Protective Shutdown" dialogs.

Dirk

Report · Sep 08, 2010

Hi Dirk,

Agreed - I also would like to see more real world source code that explains concepts. It would be good to see samples geared towards operations on a very small set of types (i.e. Libraries, Library Commands) with tutorials explaining what the sample is trying to demonstrate. For example:

Library sample demostrates:

How to open a library.
How to close a library.
How to add items via approach A.
How to add items via approach B.
How to remove an item from a library.
How to remove an item from a library based on a specific criteria.
How to remove all items from a library.

Such samples would also demonstrate Adobe's idea of best practice - consistent code style, comments and error handling.

As Helmut25 posts in his thread "Tutorial for plug-in programming?" it would be good to see more step by step tutorials that don't just do a copy and paste but also explain clearly what is being done and why. You want to get into an Adobe developers head, understand their view of the universe and then apply what you've learned in your own projects.

Back onto error handling...

I've spent many a year doing Win32 programming (via C-style API, not MFC), so I'm used to using GetLastError() to find out why a function failed. Never really had reason to use SetLastError() and by and large avoided it. I think any API has to state what will happen when things go wrong (Microsoft's Win32 documentation is very good at this) and consequently you know how to write your code to account for such things.

One thing I like to do on standalone Win32/C/C++ applications is use a tool called AppVerifier, which allows you to throw various spanners into the works and see how your application copes. Shame there isn't something similar for testing plug-ins. I like to think that if InDesign crashes, it's not down to my code.

Regards,

APMABC