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

[Behdad Esfahbod]

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;
> ...
> 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.

Why?


> 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
thing.

A transactional API would look like:

  cairo_begin()
  some drawing
  some more drawing
  cairo_rollback()

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.


-- 
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