[cairo] 'cairo_restore()' doesn't restore context in error case
Iosif Haidu
iosif.haidu at gmail.com
Tue Feb 19 13:00:15 PST 2008
On Tue, Feb 19, 2008 at 3:32 AM, Behdad Esfahbod <behdad at behdad.org> wrote:
> 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).
>
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.
Do you consider that windows device context handle is not worth to be
restored ? ('dc' member of 'cairo_win32_surface_t')
>
> >> '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.
>
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.
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. 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:
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.
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.
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.
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)
> 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.
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.
>
> >> 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.
If you will continue to follow the code execution of those examples I gave,
you'll see that actually the fields of cairo context are not damaged or set
to an inconsistent values. Just follow the execution path.
>
>
>
> >> 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.
Well I disagree with your solution for the following reason:
- it's not easy to implement it
- it's not wise to let the developer to worry when it needs to reset the
error code or not. Another thing to worry about.
>
>
>
> >> 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.
>
Actually, yes. Resetting their status to SUCCESS will magically make them
continue working, because nothing is damaged in the cairo context structure
if a windows gdi function fails.
>
>
> > }
> > }
> > // 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
>
>
Best regards,
Iosif Haidu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.cairographics.org/archives/cairo/attachments/20080219/fe7ff6d0/attachment-0001.html
More information about the cairo
mailing list