From b22e82ad578c835996e184cefe5a26a70378714a Mon Sep 17 00:00:00 2001 From: Yonit Halperin Date: Tue, 8 Jan 2013 10:47:53 -0500 Subject: [PATCH spice 2/6] red_worker.c: insert a drawable to its position in the current tree before calling red_detach_streams_behind resolves: rhbz#891326 Starting from commit 81fe00b08ad4f, red_detach_streams_behind can trigger modifications in the current tree (by update_area calls). Thus, after calling red_detach_streams_behind it is not safe to access tree entries that were calculated before the call. This patch inserts the drawable to the tree before the call to red_detach_streams_behind. This change also requires making sure that rendering operations that can be triggered by red_detach_streams_behind will not include this drawable (which is now part of the tree). --- server/red_worker.c | 55 ++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 42 insertions(+), 13 deletions(-) diff --git a/server/red_worker.c b/server/red_worker.c index 1a9c375..5e00cb6 100644 --- a/server/red_worker.c +++ b/server/red_worker.c @@ -1022,6 +1022,8 @@ static void red_current_flush(RedWorker *worker, int surface_id); #else static void red_draw_drawable(RedWorker *worker, Drawable *item); static void red_update_area(RedWorker *worker, const SpiceRect *area, int surface_id); +static void red_update_area_till(RedWorker *worker, const SpiceRect *area, int surface_id, + Drawable *last); #endif static void red_release_cursor(RedWorker *worker, CursorItem *cursor); static inline void release_drawable(RedWorker *worker, Drawable *item); @@ -2615,7 +2617,9 @@ static int red_display_drawable_is_in_pipe(DisplayChannelClient *dcc, Drawable * * after red_display_detach_stream_gracefully is called for all the display channel clients, * red_detach_stream should be called. See comment (1). */ -static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dcc, Stream *stream) +static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dcc, + Stream *stream, + Drawable *update_area_limit) { int stream_id = get_stream_id(dcc->common.worker, stream); StreamAgent *agent = &dcc->stream_agents[stream_id]; @@ -2669,27 +2673,41 @@ static inline void red_display_detach_stream_gracefully(DisplayChannelClient *dc spice_debug("stream %d: upgrade by screenshot. has current %d. box ==>", stream_id, stream->current != NULL); rect_debug(&upgrade_area); - red_update_area(dcc->common.worker, &upgrade_area, 0); + if (update_area_limit) { + red_update_area_till(dcc->common.worker, &upgrade_area, 0, update_area_limit); + } else { + red_update_area(dcc->common.worker, &upgrade_area, 0); + } red_add_surface_area_image(dcc, 0, &upgrade_area, NULL, FALSE); } } -static inline void red_detach_stream_gracefully(RedWorker *worker, Stream *stream) +static inline void red_detach_stream_gracefully(RedWorker *worker, Stream *stream, + Drawable *update_area_limit) { RingItem *item; DisplayChannelClient *dcc; WORKER_FOREACH_DCC(worker, item, dcc) { - red_display_detach_stream_gracefully(dcc, stream); + red_display_detach_stream_gracefully(dcc, stream, update_area_limit); } if (stream->current) { red_detach_stream(worker, stream, TRUE); } } -// region should be a primary surface region -static void red_detach_streams_behind(RedWorker *worker, QRegion *region) +/* + * region : a primary surface region. Streams that intersects with the given + * region will be detached. + * drawable: If detaching the stream is triggered by the addition of a new drawable + * that is dependent on the given region, and the drawable is already a part + * of the "current tree", the drawable parameter should be set with + * this drawable, otherwise, it should be NULL. Then, if detaching the stream + * involves sending an upgrade image to the client, this drawable won't be rendered + * (see red_display_detach_stream_gracefully). + */ +static void red_detach_streams_behind(RedWorker *worker, QRegion *region, Drawable *drawable) { Ring *ring = &worker->streams; RingItem *item = ring_get_head(ring); @@ -2706,7 +2724,7 @@ static void red_detach_streams_behind(RedWorker *worker, QRegion *region) StreamAgent *agent = &dcc->stream_agents[get_stream_id(worker, stream)]; if (region_intersects(&agent->vis_region, region)) { - red_display_detach_stream_gracefully(dcc, stream); + red_display_detach_stream_gracefully(dcc, stream, drawable); detach_stream = 1; spice_debug("stream %d", get_stream_id(worker, stream)); } @@ -2798,7 +2816,7 @@ static inline void red_handle_streams_timout(RedWorker *worker) Stream *stream = SPICE_CONTAINEROF(item, Stream, link); item = ring_next(ring, item); if (now >= (stream->last_time + RED_STREAM_TIMOUT)) { - red_detach_stream_gracefully(worker, stream); + red_detach_stream_gracefully(worker, stream, NULL); red_stop_stream(worker, stream); } } @@ -3507,13 +3525,24 @@ static inline int red_current_add(RedWorker *worker, Ring *ring, Drawable *drawa exclude_region(worker, ring, exclude_base, &exclude_rgn, NULL, drawable); red_use_stream_trace(worker, drawable); red_streams_update_visible_region(worker, drawable); + /* + * Performing the insertion after exclude_region for + * safety (todo: Not sure if exclude_region can affect the drawable + * if it is added to the tree before calling exclude_region). + */ + __current_add_drawable(worker, drawable, ring); } else { + /* + * red_detach_streams_behind can affect the current tree since it may + * trigger calls to update_area. Thus, the drawable should be added to the tree + * before calling red_detach_streams_behind + */ + __current_add_drawable(worker, drawable, ring); if (drawable->surface_id == 0) { - red_detach_streams_behind(worker, &drawable->tree_item.base.rgn); + red_detach_streams_behind(worker, &drawable->tree_item.base.rgn, drawable); } } region_destroy(&exclude_rgn); - __current_add_drawable(worker, drawable, ring); stat_add(&worker->add_stat, start_time); return TRUE; } @@ -3567,7 +3596,7 @@ static inline int red_current_add_with_shadow(RedWorker *worker, Ring *ring, Dra // only primary surface streams are supported if (is_primary_surface(worker, item->surface_id)) { - red_detach_streams_behind(worker, &shadow->base.rgn); + red_detach_streams_behind(worker, &shadow->base.rgn, NULL); } ring_add(ring, &shadow->base.siblings_link); __current_add_drawable(worker, item, ring); @@ -3579,7 +3608,7 @@ static inline int red_current_add_with_shadow(RedWorker *worker, Ring *ring, Dra red_streams_update_visible_region(worker, item); } else { if (item->surface_id == 0) { - red_detach_streams_behind(worker, &item->tree_item.base.rgn); + red_detach_streams_behind(worker, &item->tree_item.base.rgn, item); } } stat_add(&worker->add_stat, start_time); @@ -3911,7 +3940,7 @@ static inline int red_handle_surfaces_dependencies(RedWorker *worker, Drawable * QRegion depend_region; region_init(&depend_region); region_add(&depend_region, &drawable->red_drawable->surfaces_rects[x]); - red_detach_streams_behind(worker, &depend_region); + red_detach_streams_behind(worker, &depend_region, NULL); } } } -- 1.8.1