Introduce "killed" state for xwidgets (Re: shr using `make-xwidget' incorrectly)

[Po Lu <luangruo@yahoo.com>]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> I think the best thing to do in this case would be to introduce a
>> `dead' state for xwidgets, not unlike killed buffers and dead frames,
>> which an xwidget is set to after being killed.
>>
>> Then, performing any sort of operation on a killed xwidget can either
>> throw an error, or do nothing.

> Sounds good to me.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Prevent-crashes-in-xwidgets-whose-buffers-have-been-.patch --]
[-- Type: text/x-patch, Size: 12941 bytes --]

From 7db8a74c8eb463db9b4db20fb5c3964b28f1c521 Mon Sep 17 00:00:00 2001
From: Po Lu <luangruo@yahoo.com>
Date: Thu, 11 Nov 2021 14:29:12 +0800
Subject: [PATCH] Prevent crashes in xwidgets whose buffers have been killed

* doc/lispref/display.texi (Xwidgets): Explain meaning of killed
xwidgets.
* src/xwidget.c (Fxwidget_live_p): New function.
(Fxwidget_perform_lispy_event, WEBKIT_FN_INIT)
(Fxwidget_resize, Fxwidget_size_request)
(Fxwidget_info, Fxwidget_plist)
(Fset_xwidget_buffer, Fset_xwidget_plist)
(Fset_xwidget_query_on_exit_flag)
(Fxwidget_query_on_exit_flag)
(Fxwidget_webkit_search)
(Fxwidget_webkit_next_result)
(Fxwidget_webkit_previous_result)
(Fxwidget_webkit_finish_search)
(Fxwidget_webkit_load_html): Check for live xwidgets instead of
just xwidgets.

(xwidget_button, xwidget_motion_or_crossing)
(xv_do_draw, x_draw_xwidget_glyph_string)
(Fdelete_xwidget_view): Ignore killed xwidgets.

(syms_of_xwidget): Define new symbols and subrs and define
appropriate weakness of id_to_xwidget map.
(kill_buffer_xwidgets): Check live xwidgets instead of
killed xwidgets, set xwidget buffer to nil, and rely on GC to
free the hash table for us instead.

* src/xwidget.h (XWIDGET_LIVE_P, CHECK_LIVE_XWIDGET): New
macros.
---
 doc/lispref/display.texi |  14 ++++-
 src/xwidget.c            | 116 +++++++++++++++++++++++++++------------
 src/xwidget.h            |   7 +++
 3 files changed, 101 insertions(+), 36 deletions(-)

diff --git a/doc/lispref/display.texi b/doc/lispref/display.texi
index 094bb5a5f3..63d2838eaa 100644
--- a/doc/lispref/display.texi
+++ b/doc/lispref/display.texi
@@ -6804,6 +6804,12 @@ Xwidgets
 is used internally by the WebKit widget, and specifies another WebKit
 widget that the newly created widget should share settings and
 subprocesses with.
+
+The xwidget that is returned will be killed alongside its buffer
+(@pxref{Killing Buffers}).  Once it is killed, the xwidget may
+continue to exist as a Lisp object and act as a @code{display}
+property until all references to it are gone, but most actions that
+can be performed on live xwidgets will no longer be available.
 @end defun
 
 @defun xwidgetp object
@@ -6811,6 +6817,11 @@ Xwidgets
 @code{nil} otherwise.
 @end defun
 
+@defun xwidget-live-p object
+This function returns @code{t} if @var{object} is an xwidget that
+hasn't been killed, and @code{nil} otherwise.
+@end defun
+
 @defun xwidget-plist xwidget
 This function returns the property list of @var{xwidget}.
 @end defun
@@ -6821,7 +6832,8 @@ Xwidgets
 @end defun
 
 @defun xwidget-buffer xwidget
-This function returns the buffer of @var{xwidget}.
+This function returns the buffer of @var{xwidget}.  If @var{xwidget}
+has been killed, it returns @code{nil}.
 @end defun
 
 @defun set-xwidget-buffer xwidget buffer
diff --git a/src/xwidget.c b/src/xwidget.c
index ace63e3742..4c18c531b9 100644
--- a/src/xwidget.c
+++ b/src/xwidget.c
@@ -273,6 +273,16 @@ DEFUN ("make-xwidget",
   return val;
 }
 
+DEFUN ("xwidget-live-p", Fxwidget_live_p, Sxwidget_live_p,
+       1, 1, 0, doc: /* Return t if OBJECT is an xwidget that has not been killed.
+Value is nil if OBJECT is not an xwidget or if it has been killed.  */)
+  (Lisp_Object object)
+{
+  return ((XWIDGETP (object)
+	   && !NILP (XXWIDGET (object)->buffer))
+	  ? Qt : Qnil);
+}
+
 #ifdef USE_GTK
 static void
 set_widget_if_text_view (GtkWidget *widget, void *data)
@@ -305,7 +315,7 @@ DEFUN ("xwidget-perform-lispy-event",
   GtkWidget *temp = NULL;
 #endif
 
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   xw = XXWIDGET (xwidget);
 
   if (!NILP (frame))
@@ -807,6 +817,9 @@ xwidget_button (struct xwidget_view *view,
 		bool down_p, int x, int y, int button,
 		int modifier_state, Time time)
 {
+  if (NILP (XXWIDGET (view->model)->buffer))
+    return;
+
   record_osr_embedder (view);
 
   if (button < 4 || button > 8)
@@ -857,22 +870,29 @@ xwidget_button (struct xwidget_view *view,
 void
 xwidget_motion_or_crossing (struct xwidget_view *view, const XEvent *event)
 {
-  GdkEvent *xg_event = gdk_event_new (event->type == MotionNotify
-				      ? GDK_MOTION_NOTIFY
-				      : (event->type == LeaveNotify
-					 ? GDK_LEAVE_NOTIFY
-					 : GDK_ENTER_NOTIFY));
+  GdkEvent *xg_event;
   struct xwidget *model = XXWIDGET (view->model);
   int x;
   int y;
-  GtkWidget *target = find_widget_at_pos (model->widgetwindow_osr,
-					  (event->type == MotionNotify
-					   ? event->xmotion.x + view->clip_left
-					   : event->xcrossing.x + view->clip_left),
-					  (event->type == MotionNotify
-					   ? event->xmotion.y + view->clip_top
-					   : event->xcrossing.y + view->clip_top),
-					  &x, &y);
+  GtkWidget *target;
+
+  if (NILP (model->buffer))
+    return;
+
+  xg_event = gdk_event_new (event->type == MotionNotify
+			    ? GDK_MOTION_NOTIFY
+			    : (event->type == LeaveNotify
+			       ? GDK_LEAVE_NOTIFY
+			       : GDK_ENTER_NOTIFY));
+
+  target = find_widget_at_pos (model->widgetwindow_osr,
+			       (event->type == MotionNotify
+				? event->xmotion.x + view->clip_left
+				: event->xcrossing.x + view->clip_left),
+			       (event->type == MotionNotify
+				? event->xmotion.y + view->clip_top
+				: event->xcrossing.y + view->clip_top),
+			       &x, &y);
 
   if (!target)
     target = model->widget_osr;
@@ -969,6 +989,13 @@ xv_do_draw (struct xwidget_view *xw, struct xwidget *w)
 {
   GtkOffscreenWindow *wnd;
   cairo_surface_t *surface;
+
+  if (NILP (w->buffer))
+    {
+      XClearWindow (xw->dpy, xw->wdesc);
+      return;
+    }
+
   block_input ();
   wnd = GTK_OFFSCREEN_WINDOW (w->widgetwindow_osr);
   surface = gtk_offscreen_window_get_surface (wnd);
@@ -1651,14 +1678,25 @@ x_draw_xwidget_glyph_string (struct glyph_string *s)
      a redraw.  It seems its possible to get out of sync with emacs
      redraws so emacs background sometimes shows up instead of the
      xwidgets background.  It's just a visual glitch though.  */
-  if (!xwidget_hidden (xv))
+  /* When xww->buffer is nil, that means the xwidget has been killed.  */
+  if (!NILP (xww->buffer))
     {
+      if (!xwidget_hidden (xv))
+	{
 #ifdef USE_GTK
-      gtk_widget_queue_draw (xww->widget_osr);
+	  gtk_widget_queue_draw (xww->widget_osr);
 #elif defined NS_IMPL_COCOA
-      nsxwidget_set_needsdisplay (xv);
+	  nsxwidget_set_needsdisplay (xv);
 #endif
+	}
+    }
+#ifdef USE_GTK
+  else
+    {
+      XSetWindowBackground (xv->dpy, xv->wdesc,
+			    FRAME_BACKGROUND_PIXEL (s->f));
     }
+#endif
 
 #ifdef USE_GTK
   unblock_input ();
@@ -1677,7 +1715,7 @@ xwidget_is_web_view (struct xwidget *xw)
 
 /* Macro that checks xwidget hold webkit web view first.  */
 #define WEBKIT_FN_INIT()						\
-  CHECK_XWIDGET (xwidget);						\
+  CHECK_LIVE_XWIDGET (xwidget);						\
   struct xwidget *xw = XXWIDGET (xwidget);				\
   if (!xwidget_is_web_view (xw))					\
     {									\
@@ -1856,7 +1894,7 @@ DEFUN ("xwidget-resize", Fxwidget_resize, Sxwidget_resize, 3, 3, 0,
        doc: /* Resize XWIDGET to NEW_WIDTH, NEW_HEIGHT.  */ )
   (Lisp_Object xwidget, Lisp_Object new_width, Lisp_Object new_height)
 {
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   int w = check_integer_range (new_width, 0, INT_MAX);
   int h = check_integer_range (new_height, 0, INT_MAX);
   struct xwidget *xw = XXWIDGET (xwidget);
@@ -1907,7 +1945,7 @@ DEFUN ("xwidget-size-request",
 Emacs allocated area accordingly.  */)
   (Lisp_Object xwidget)
 {
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
 #ifdef USE_GTK
   GtkRequisition requisition;
   gtk_widget_size_request (XXWIDGET (xwidget)->widget_osr, &requisition);
@@ -1942,7 +1980,7 @@ DEFUN ("xwidget-info",
 Currently [TYPE TITLE WIDTH HEIGHT].  */)
   (Lisp_Object xwidget)
 {
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   struct xwidget *xw = XXWIDGET (xwidget);
   return CALLN (Fvector, xw->type, xw->title,
 		make_fixed_natnum (xw->width), make_fixed_natnum (xw->height));
@@ -2005,7 +2043,7 @@ DEFUN ("delete-xwidget-view",
       unblock_input ();
     }
 
-  if (xw->embedder_view == xv)
+  if (xw->embedder_view == xv && !NILP (xw->buffer))
     {
       w = gtk_widget_get_window (xw->widgetwindow_osr);
 
@@ -2054,7 +2092,7 @@ DEFUN ("xwidget-plist",
        doc: /* Return the plist of XWIDGET.  */)
   (Lisp_Object xwidget)
 {
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   return XXWIDGET (xwidget)->plist;
 }
 
@@ -2074,7 +2112,7 @@ DEFUN ("set-xwidget-buffer",
        doc: /* Set XWIDGET's buffer to BUFFER.  */)
   (Lisp_Object xwidget, Lisp_Object buffer)
 {
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   CHECK_BUFFER (buffer);
 
   XXWIDGET (xwidget)->buffer = buffer;
@@ -2088,7 +2126,7 @@ DEFUN ("set-xwidget-plist",
 Returns PLIST.  */)
   (Lisp_Object xwidget, Lisp_Object plist)
 {
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   CHECK_LIST (plist);
 
   XXWIDGET (xwidget)->plist = plist;
@@ -2104,7 +2142,7 @@ DEFUN ("set-xwidget-query-on-exit-flag",
 This function returns FLAG.  */)
   (Lisp_Object xwidget, Lisp_Object flag)
 {
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   XXWIDGET (xwidget)->kill_without_query = NILP (flag);
   return flag;
 }
@@ -2115,7 +2153,7 @@ DEFUN ("xwidget-query-on-exit-flag",
        doc: /* Return the current value of the query-on-exit flag for XWIDGET.  */)
   (Lisp_Object xwidget)
 {
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   return (XXWIDGET (xwidget)->kill_without_query ? Qnil : Qt);
 }
 
@@ -2148,7 +2186,7 @@ DEFUN ("xwidget-webkit-search", Fxwidget_webkit_search, Sxwidget_webkit_search,
 #endif
 
   CHECK_STRING (query);
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
 
 #ifdef USE_GTK
   xw = XXWIDGET (xwidget);
@@ -2192,7 +2230,7 @@ DEFUN ("xwidget-webkit-next-result", Fxwidget_webkit_next_result,
   WebKitFindController *controller;
 #endif
 
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   xw = XXWIDGET (xwidget);
 
   if (!xw->find_text)
@@ -2224,7 +2262,7 @@ DEFUN ("xwidget-webkit-previous-result", Fxwidget_webkit_previous_result,
   WebKitFindController *controller;
 #endif
 
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   xw = XXWIDGET (xwidget);
 
   if (!xw->find_text)
@@ -2256,7 +2294,7 @@ DEFUN ("xwidget-webkit-finish-search", Fxwidget_webkit_finish_search,
   WebKitFindController *controller;
 #endif
 
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   xw = XXWIDGET (xwidget);
 
   if (!xw->find_text)
@@ -2294,7 +2332,7 @@ DEFUN ("xwidget-webkit-load-html", Fxwidget_webkit_load_html,
   WebKitWebView *webview;
   char *data, *uri;
 
-  CHECK_XWIDGET (xwidget);
+  CHECK_LIVE_XWIDGET (xwidget);
   CHECK_STRING (text);
   if (NILP (base_uri))
     base_uri = build_string ("about:blank");
@@ -2415,7 +2453,9 @@ syms_of_xwidget (void)
 {
   defsubr (&Smake_xwidget);
   defsubr (&Sxwidgetp);
+  defsubr (&Sxwidget_live_p);
   DEFSYM (Qxwidgetp, "xwidgetp");
+  DEFSYM (Qxwidget_live_p, "xwidget-live-p");
   defsubr (&Sxwidget_view_p);
   DEFSYM (Qxwidget_view_p, "xwidget-view-p");
   defsubr (&Sxwidget_info);
@@ -2474,7 +2514,8 @@ syms_of_xwidget (void)
 
   Fprovide (intern ("xwidget-internal"), Qnil);
 
-  id_to_xwidget_map = CALLN (Fmake_hash_table, QCtest, Qeq);
+  id_to_xwidget_map = CALLN (Fmake_hash_table, QCtest, Qeq,
+			     QCweakness, Qvalue);
   staticpro (&id_to_xwidget_map);
 
 #ifdef USE_GTK
@@ -2700,9 +2741,10 @@ kill_buffer_xwidgets (Lisp_Object buffer)
       Vxwidget_list = Fdelq (xwidget, Vxwidget_list);
       /* TODO free the GTK things in xw.  */
       {
-        CHECK_XWIDGET (xwidget);
+        CHECK_LIVE_XWIDGET (xwidget);
         struct xwidget *xw = XXWIDGET (xwidget);
-	Fremhash (make_fixnum (xw->xwidget_id), id_to_xwidget_map);
+	xw->buffer = Qnil;
+
 #ifdef USE_GTK
         if (xw->widget_osr && xw->widgetwindow_osr)
           {
@@ -2719,6 +2761,10 @@ kill_buffer_xwidgets (Lisp_Object buffer)
 		xfree (xmint_pointer (XCAR (cb)));
 	      ASET (xw->script_callbacks, idx, Qnil);
 	    }
+
+	xw->widget_osr = NULL;
+	xw->widgetwindow_osr = NULL;
+	xw->find_text = NULL;
 #elif defined NS_IMPL_COCOA
         nsxwidget_kill (xw);
 #endif
diff --git a/src/xwidget.h b/src/xwidget.h
index 6e6b39c8b4..4377b50e84 100644
--- a/src/xwidget.h
+++ b/src/xwidget.h
@@ -138,9 +138,16 @@ #define XWIDGETP(x) PSEUDOVECTORP (x, PVEC_XWIDGET)
 #define XXWIDGET(a) (eassert (XWIDGETP (a)), \
 		     XUNTAG (a, Lisp_Vectorlike, struct xwidget))
 
+#define XWIDGET_LIVE_P(w) (!NILP ((w)->buffer))
+
 #define CHECK_XWIDGET(x) \
   CHECK_TYPE (XWIDGETP (x), Qxwidgetp, x)
 
+#define CHECK_LIVE_XWIDGET(x)				\
+  CHECK_TYPE ((XWIDGETP (x)				\
+	       && XWIDGET_LIVE_P (XXWIDGET (x))),	\
+	      Qxwidget_live_p, x)
+
 /* Test for xwidget_view pseudovector.  */
 #define XWIDGET_VIEW_P(x) PSEUDOVECTORP (x, PVEC_XWIDGET_VIEW)
 #define XXWIDGET_VIEW(a) (eassert (XWIDGET_VIEW_P (a)), \
-- 
2.31.1


[-- Attachment #3: Type: text/plain, Size: 112 bytes --]


Thanks, how does this look to you?  And perhaps Eli has something to
say about it as well.

Thanks in advance.