[cairo] 'cairo_restore()' doesn't restore context in error case

[Behdad Esfahbod]

On Sat, 2008-02-16 at 00:50 +0200, Iosif Haidu wrote:
> Hi,

Hi,

> I have recently experienced an interesting behavior of the
> 'cairo_restore()' function. It doesn't restore the cairo context saved
> by 'cairo_save()' if an error occurs when executing windows gdi
> functions.
> 
> When I used these two functions I assumed that they must provide a
> transaction like functionality:

Wrong assumption.  And of course wrong documentation on our side.  What
cairo_save/restore do is save/restore all the settings/parameters of the
context except for groups, current path, and of course status.  In
postscript speak, it saves/restores the graphics state (sans the path).


>  'cairo_save()' saves the cairo context while 'cairo_restore()' would
> restore it even if an error would occur by executing cairo functions
> between. But this is not the case. If a cairo function fail to execute
> between,  by failing one of the windows gdi function, then
> 'cairo_restore()' will not restore the previously saved context. 
> I have experienced such situation for the application I'm working on
> it. For an unknown reason sometime some windows gdi functions
> (CreateCompatibleDC(), BitBlt() and other) fail to execute with error
> code 6 and 8. In this situation the cairo function responsible with
> that windows gdi function call will set the error status to
> CAIRO_STATUS_NO_MEMORY and the 'cairo_restore()' will not restore the
> cairo context. I have noticed that cairo context actually has not been
> damaged by the failure of the windows gdi function, yet the context is
> not restored.

As I said on the bug, the this is a bug that should be fixed.  If the
failing gdi functions are not fatal, this should be handled in cairo.

If the errors are fatal errors (say, real out-of-memory), cairo fails to
produce the exact output that you asked it to, and that's an error.  And
cairo's error handling model is to retain errors.  It's really that
simple.  What is it good for if cairo tells you that everything is ok
but in reality it failed to draw some of the stuff you asked it to?


> Also the cairo documentation doesn't mention that 'cairo_restore()'
> will not restore the context if an error occurs in a cairo function
> between 'cairo_save()' and 'cairo_restore()'.

When a cairo object is in error, it shuts down.  No further operation on
it works normally.  This is to ensure that no malicious behavior is
caused by an erroneous object.


> I would like to propose a change in the 'cairo_restore()'
> implementation, such that:
>   a) will restore the previously saved context by 'cairo_save()' even
> if in an error occurs
>   b) will return as result the error code generated by any cairo
> function executed between 'cairo_save()' and 'cairo_restore()'
>   c) will reset the error status to CAIRO_STATUS_SUCCESS so that any
> next execution of 'cairo_save()' and 'cairo_restore()' will not exit
> because of a previous status set to an error code.

So, no.  This doesn't make any sense to me.  What does make some sense
is to add some new api, for example cairo_clear_status().  But
implementing that kind of API is almost impossible without really
drastic changes to cairo internals.

I do see where you are coming from though.  I was myself very surprised
and puzzled by the fact that this can put the cairo_t in a
non-recoverable error state:

  cairo_save (cr);
  cairo_scale (cr, 0, 0);
  cairo_restore (cr);

But my solution to it is not what suggest.  My proposed solution is that
errors should not be generated until there's really no other choice.  In
the above case, there's nothing wrong with the code semantically.  For
one reason, it's not drawing anything with the degenerate matrix, so it
can't be an error.  Moreover, drawing with a degenerate matrix is quite
well-defined and useful too.  So, these cases are false positives in
current cairo implementation that should be fixed, instead of hacked
around.  From what you described, your case is similar IMO.


> The function would be something like:
> 
> int
> cairo_restore (cairo_t *cr)
> {
> ...
> int result = CAIRO_STATUS_SUCESS;
> if (cr->status != CAIRO_STATUS_SUCCESS)
> {
>     result = cr->status;
>     cr->status = CAIRO_STATUS_SUCCESS;
>     if ((cr->gstate) && (cr->gstate->target))
>     {
>         cr->gstate->target->status = CAIRO_STATUS_SUCCESS;

Any and all of cr, cr->gstate, and cr->gstate->target may be in an
inconsistent state at this point.  Reseting their status to SUCCESS
doesn't magically make them continue working.


>     }
> }
> // if (cr->status)
> // return;
> ...
> return result;
> }
> 
> 
> Best regards,
> Iosif Haidu

-- 
behdad
http://behdad.org/

"Those who would give up Essential Liberty to purchase a little
 Temporary Safety, deserve neither Liberty nor Safety."
        -- Benjamin Franklin, 1759