wayland-client: reuse the existing map entry for the new_id of object from server

This change will make a wayland client reuse an existing map entry for the new id
of object in an event coming from server. To sum up, this is an error recovery code
that occurs when a wayland client doesn't destroy both the server-side resource and
the client-side proxy object in critical section that is guaranteed with the display
lock(mutex).

There is an error about the reservation of an entry in server_entries of the map of
a client. It will be caused when there is data of a wl_object stored already in the
map entry associated with the new object id coming from the server as an event.
Actually this issue will happen in a multi-threaded wayland client. Now let me give
you an example associate with a wl_buffer object created in the display server.

Suppose there are two threads in the client process. A thread(TA) is dispatching
events in its event queue and the other thread(TB) called wl_display_prepare_read()
to prepare for reading the display. And then thread(TA) called wl_buffer_destroy()
to destroy a wl_buffer object and then it finished to send the "WL_BUFFER_DESTROY"
request to the server via wl_proxy_marshal(). At the same time, thread(TB) polled
the display fd, returned from the polling function by the events coming from the
server and then called wl_display_read_events() to read/queue events.

In this situation, thread(TB) will be waiting for getting the display lock(mutex)
and will get it soon as there is no threads which holds the display lock.
thread(TB) will soon read events from the display fd and will queue the events
to the queue(s). At the same time, thread(TA) will call wl_proxy_destroy() to
destroy the proxy object, it'll try to hold the display lock. But it'll stop at the
part where it waits for the lock because thread(TB) holds the lock.

In the server side, it'll release the wl_buffer resource associate with the
id given from the destroy request from the client. And then the id will be reused
when it creates a new wl_buffer resource and will send the event contains the id as
a new_id of object. While thread(TB) is reading events and creating/queuing events,
the event from the server will also be handled.

In the process of demarshalling an event and calling wl_map_reserve_new() to reserve
a map entry for a new id of object on the map, the wayland client sets an error(EINVAL).
It's because the client finds that there is already data in the map entry corresponding
to the new object id coming from the server. Once the error is set, the error value will
be set to display->last_error and then the wayland client doesn't work properly.

In this change of code, the additional flag(WL_PROXY_FLAG_ID_OVERRIDEN) for wl_proxy
object will be appeared newly. When a new object id that is already associated with the
map entry where data exists come from the server, the wayland client adds the flag
(WL_PROXY_FLAG_ID_OVERRIDEN) to the proxy object stored int he map entry.
And this allows only the proxy object itself to be destroyed without destroying the data
of the linked map entry when the proxy object is destroyed in the future. Of course,
the data (in the map entry associated with the object id) has been being reused for a new
proxy object created in queue_event().

Change-Id: I62e762e94d081ef3d2ce2f54a0cc5d54c371327c
Signed-off-by: Sung-Jin Park <sj76.park@samsung.com>

diff --git a/src/connection.c b/src/connection.c
index a9f8de7..8f3223b 100644 (file)
--- a/src/connection.c
+++ b/src/connection.c
@@ -869,6 +869,9 @@ wl_connection_demarshal(struct wl_connection *connection,
        struct wl_closure *closure;
        struct wl_array *array_extra;
 
+       int res;
+       void *object_to_override;
+
        /* Space for sender_id and opcode */
        if (size < 2 * sizeof *p) {
                wl_log("message too short, invalid header\n");
@@ -980,6 +983,20 @@ wl_connection_demarshal(struct wl_connection *connection,
                                goto err;
                        }
 
+                       /* Check if there is any object data exists with the given id */
+                       object_to_override = wl_map_lookup(objects, id);
+
+                       if (object_to_override)
+                       {
+                               res = wl_display_override_object_id(object_to_override, id, objects->side);
+
+                               if (res)
+                                       wl_log("id (%u) has been overriden for new object, "
+                                              "message %s(%s)\n",
+                                              id, message->name, message->signature);
+                               break;
+                       }
+
                        if (wl_map_reserve_new(objects, id) < 0) {
                                if (errno == EINVAL) {
                                        wl_log("not a valid new object id (%u), "

diff --git a/src/wayland-client.c b/src/wayland-client.c
index dfb322f..41fa711 100644 (file)
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -63,6 +63,7 @@ enum wl_proxy_flag {
        WL_PROXY_FLAG_ID_DELETED = (1 << 0),
        WL_PROXY_FLAG_DESTROYED = (1 << 1),
        WL_PROXY_FLAG_WRAPPER = (1 << 2),
+       WL_PROXY_FLAG_ID_OVERRIDEN = (1 << 31)
 };
 
 struct wl_zombie {
@@ -586,8 +587,11 @@ proxy_destroy(struct wl_proxy *proxy)
                                 proxy->object.id,
                                 zombie);
        } else {
-               wl_map_insert_at(&proxy->display->objects, 0,
-                                proxy->object.id, NULL);
+               /* Clear the given proxy information from the map
+                * only if it is not overriden. */
+               if (!(proxy->flags & WL_PROXY_FLAG_ID_OVERRIDEN))
+                       wl_map_insert_at(&proxy->display->objects, 0,
+                                        proxy->object.id, NULL);
        }
 
        proxy->flags |= WL_PROXY_FLAG_DESTROYED;
@@ -1740,6 +1744,46 @@ increase_closure_args_refcount(struct wl_closure *closure)
        closure->proxy->refcount++;
 }
 
+int
+wl_display_override_object_id(void *object, const uint32_t id, int side)
+{
+       struct wl_proxy *proxy = (struct wl_proxy *)object;
+       struct wl_proxy *overriden_proxy;
+
+       /* Check if the given object is the pointer of wl_proxy object */
+       if (!proxy || (sizeof(*proxy) != sizeof(struct wl_proxy))) {
+               wl_log("errno:%d\n", errno);
+               wl_abort("invalid object data to override (id:%u, errno:%d)\n", id, errno = EINVAL);
+               return 0;
+       }
+       else if (id < WL_SERVER_ID_START || side != WL_MAP_CLIENT_SIDE) {
+               wl_log("errno:%d\n", errno);
+               wl_abort("invalid object id to override (id:%u, side:%d, errno:%d)\n", id, side, errno = EINVAL);
+               return 0;
+       }
+
+       overriden_proxy = wl_map_lookup(&proxy->display->objects, id);
+       if (!overriden_proxy) {
+               if (errno || proxy->display->last_error)
+                       wl_log("errno:%d, last_error:%d\n", errno, proxy->display->last_error);
+               wl_abort("no proxy found with the given object id(%u), errno(%d)\n", id, errno = EINVAL);
+               return 0;
+       }
+       else if (overriden_proxy != proxy) {
+               if (errno || proxy->display->last_error)
+                       wl_log("errno:%d, last_error:%d\n", errno, proxy->display->last_error);
+               wl_abort("invalid proxy objects(%p, %p) found (id:%u), errno(%d)\n", overriden_proxy, proxy, id, errno = EINVAL);
+               return 0;
+       }
+
+       /* Set overriden flag for the overriden proxy not to set data as NULL later. */
+       overriden_proxy->flags |= WL_PROXY_FLAG_ID_OVERRIDEN;
+       overriden_proxy->object.id = 0;
+
+       wl_log("proxy(%p) with id(%u) has been overriden\n", overriden_proxy, id);
+       return 1;
+}
+
 static int
 queue_event(struct wl_display *display, int len)
 {

diff --git a/src/wayland-private.h b/src/wayland-private.h
index 164b0ef..9cae0bd 100644 (file)
--- a/src/wayland-private.h
+++ b/src/wayland-private.h
@@ -298,4 +298,7 @@ zalloc(size_t s)
 void
 wl_connection_close_fds_in(struct wl_connection *connection, int max);
 
+extern int
+wl_display_override_object_id(void *object, const uint32_t id, int side);
+
 #endif

diff --git a/src/wayland-server.c b/src/wayland-server.c
old mode 100755 (executable)
new mode 100644 (file)
index 6e5453a..713bc3e
--- a/src/wayland-server.c
+++ b/src/wayland-server.c
@@ -150,6 +150,20 @@ struct wl_protocol_logger {
        void *user_data;
 };
 
+int
+wl_display_override_object_id(void *object, const uint32_t id, int side)
+{
+       /* Nothing needs to be done here in server. */
+       (void) object;
+       (void) id;
+       (void) side;
+
+       wl_log("errno:%d\n", errno);
+       wl_log("This case must not be happened. (object:%p, id:%u, side:%d, errno:%d)\n",
+                               object, id, side, errno = EINVAL);
+       return 0;
+}
+
 static void
 log_closure(struct wl_resource *resource,
            struct wl_closure *closure, int send)

diff --git a/tests/connection-test.c b/tests/connection-test.c
index eea9287..a15a3b5 100644 (file)
--- a/tests/connection-test.c
+++ b/tests/connection-test.c
@@ -43,6 +43,17 @@
 
 static const char message[] = "Hello, world";
 
+int
+wl_display_override_object_id(void *object, const uint32_t id, int side)
+{
+       /* Nothing needs to be done here. */
+       (void) object;
+       (void) id;
+       (void) side;
+
+       return 0;
+}
+
 static struct wl_connection *
 setup(int *s)
 {

diff --git a/tests/message-test.c b/tests/message-test.c
index 4e69392..7c675a7 100644 (file)
--- a/tests/message-test.c
+++ b/tests/message-test.c
@@ -30,6 +30,17 @@
 #include "wayland-server.h"
 #include "test-runner.h"
 
+int
+wl_display_override_object_id(void *object, const uint32_t id, int side)
+{
+       /* Nothing needs to be done here. */
+       (void) object;
+       (void) id;
+       (void) side;
+
+       return 0;
+}
+
 TEST(message_version)
 {
        unsigned int i;

diff --git a/tests/os-wrappers-test.c b/tests/os-wrappers-test.c
index 8d8c3ab..ecbced0 100644 (file)
--- a/tests/os-wrappers-test.c
+++ b/tests/os-wrappers-test.c
@@ -45,6 +45,17 @@
 #include "test-runner.h"
 #include "wayland-os.h"
 
+int
+wl_display_override_object_id(void *object, const uint32_t id, int side)
+{
+       /* Nothing needs to be done here. */
+       (void) object;
+       (void) id;
+       (void) side;
+
+       return 0;
+}
+
 static int fall_back;
 
 /* Play nice with sanitizers