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

Behdad Esfahbod behdad at behdad.org
Tue Feb 19 21:52:40 PST 2008

On Tue, 2008-02-19 at 23:00 +0200, Iosif Haidu wrote:

> I agree with you that my assumption might be wrong, but that's what
> happens when function names are not chose wisely (according with their
> implementation). If the 'cairo_restore()' function would have been
> named 'cairo_partial_restore()' instead, then I would have know that
> probably only some of the fields of 'cairo_t' structure will be
> restored and not all of them.

All the settings in cairo_t are restored except for the path.  But note
that I said all the *settings*.  Error status is not a setting.  You
cannot set it.

The point of save/restore is to let you set some stuff, draw, then
restore back to the previous settings.

> Do you consider that windows device context handle is not worth to be
> restored ? ('dc' member of 'cairo_win32_surface_t')

cairo_save()/cairo_restore() save/restore settings in cairo_t.  How is
that relevant to cairo_win32_surface_t?

>         > 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.  
> Well, the output of the error is that my application main window
> freezes and nothing is drawn anymore (I must close the application and
> start again). I would say that this error it's a critical one.

Ok, and I agreed that it's probably a bug that should be fixed.

> The problem why the main screen freezes is because the windows device
> context handle is not restored and somehow inside the library this is
> damaged.

What do you mean "not restored" and "damaged"?  I appreciate you trying
to analyze this, but the best way you can report a bug is to provide
code producing the bug, rather than insisting on what you think is a
proper fix.

> For an application that draws real time waveforms it would be
> acceptable to lose some drawings but not to freeze.
> Probably you didn't try to check the cairo library functions and try
> some scenario of 'what can happen if xxx windows gdi function fails
> for some unknown reason'. But I can give you some examples:

I don't have access to win32, so no, I didn't try.  But cairo on win32
is working for so many people out there.  Expecting that I try it and
see that it fails without providing me with any test code is not going
to work.

> a)
> File: cairo-win32-surface.c
> Function: static cairo_status_t _create_dc_and_bitmap(...)
> ...
> surface->dc = CreateCompatibleDC(original_dc);
> if (!surface->dc)
> goto FAIL;
> surface->bitmap = CreateDIBSection (surface->dc,
>                             bitmap_info,
>                             DIB_RGB_COLORS,
>                             &bits,
>                             NULL, 0);
> if (!surface->bitmap)
> goto FAIL;
> ...
> surface->saved_dc_bitmap = SelectObject (surface->dc,
>                          surface->bitmap);
> if (!surface->saved_dc_bitmap)
> goto FAIL;
> ...
>     status = _cairo_win32_print_gdi_error ("_create_dc_and_bitmap");
>     ...
>     if (surface->saved_dc_bitmap) {
>     SelectObject (surface->dc, surface->saved_dc_bitmap);
>     surface->saved_dc_bitmap = NULL;
>     }
> In my situation, for unknown reason, the execution of
> 'CreateCompatibleDC()' fails. I'll let you to find out what happens in
> the FAIL section if 'CreateCompatibleDC()' or 'CreateDIBSection()'
> fails.

"for unknown reason", that's where you should investigate.

> b)
> File: cairo-win32-surface.c
> Function: static cairo_status_t
> _cairo_win32_surface_acquire_dest_image()
> ...
> if (GetClipBox (surface->dc, &clip_box) == ERROR)
> return _cairo_win32_print_gdi_error
> ("_cairo_win3_surface_acquire_dest_image");
> ...
> The execution of 'GetClipBox()' sometimes fails.


> c)
> File: cairo-win32-surface.c
> Function: static cairo_int_status_t
> _cairo_win32_surface_set_clip_region()
> ...
> if (ExtSelectClipRgn (surface->dc, gdi_region, RGN_AND) == ERROR)
> status = _cairo_win32_print_gdi_error
> ("_cairo_win32_surface_set_clip_region");
> ..
> The execution of 'ExtSelectClipRgn()' sometimes fails.

Same here.

> And here are the cairo api leading to this situation:
>     cairo_save(pDcM);
>     cairo_move_to(pDcM, rBeginP.getX(), rBeginP.getY());
>     cairo_line_to(pDcM, rEndP.getX(), rEndP.getY());
>     cairo_stroke(pDcM);
>     cairo_restore(pDcM);
> Now, if in the first case it's clearly a bug to not check in the FAIL
> section if 'surface->saved_dc_bitmap' has a valid value (it has a
> garbage value != 0 and it goes inside the 'if' and that garbage value
> is selected in the surface device context) for the other two cases
> I'll let you to find out the real reason.

I can't.  But hopefully one of our win32 hackers will.  Now this is
starting to look like a real bug.  Can you please follow up with this
instead of the cairo_save()/restore() proposal?

> You could say that in the first case the bug can be fixed, so probably
> the problem will be also fixed. But fixing the 'cairo_restore()' to
> really restore what have been saved has also other benefits. For
> example such bugs will not affect anymore the main application (there
> are other mechanisms to catch such bugs if this is the intention of
> retaining the error code), the execution of a cairo_save/cairo_restore
> block will act like a transaction (transactions are a good mechanism
> to build a stable systems)

Making restore clear the error status doesn't not make it work more
robustly, no.  Transactions are a completely different beast.  Just
because the usage pattern looks similar doesn't mean these are the same

A transactional API would look like:

  some drawing
  some more drawing

at which time none of the drawing between begin and rollback will end up
in the end result.  There's currently no way to achieve that in cairo
(push/pop_group is the closest you get).  As I said, save/restore is
really something different.  Accept it.

> > 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?
> I never said that. I think you misunderstood my explanation or didn't
> pay attention enough on my previous e-mail. 

I totally understood you and paid more than enough attention.  I think
you are missing the point though: cairo_save/restore() is NOT what you
think it is.

> The developer is able to check every moment, inside of a
> 'cairo_save()/cairo_restore()' block if an error has occurred by
> calling 'cairo_status()' api function and act correspondingly.  What I
> have said was that is not necessary to keep the error but rather clear
> it after 'cairo_restore()' will finish it's execution. The only
> drawback of this fix is compatibility with previous versions. And yes,
> there might be applications that would rather like to miss some 
> drawings but instead restore the right device context handle.

I don't think discussing this further helps anyone.  I'll try to focus
on the bug at hand.


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

More information about the cairo mailing list