shr using `make-xwidget' incorrectly

shr using `make-xwidget' incorrectly

[Po Lu]

Lars, you might want to know that `make-xwidget', invoked without a
buffer argument, will create an xwidget attached to the current buffer.

That xwidget won't be killed until you kill the buffer it's attached to.
So you should either try to re-use existing xwidgets (see
`get-buffer-xwidgets'), or to kill the buffer the xwidget is attached
to.  Otherwise, every re-render will cause more xwidgets to be created,
leading to undesirable resource consumption (WebKit widgets are heavy!).

Which reminds me of another problem (in the Emacs 28 xwidget code as
well): If an xwidget's buffer is killed, but references to it still
exist somewhere, crashes can happen, because the GTK resources allocated
to the xwidget are killed in `kill_buffer_xwidgets' and not during
garbage collection.

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.

But I'd like the others to chime in as well.  Thanks.

Re: shr using `make-xwidget' incorrectly

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> Lars, you might want to know that `make-xwidget', invoked without a
> buffer argument, will create an xwidget attached to the current buffer.
>
> That xwidget won't be killed until you kill the buffer it's attached to.

Can't we fix that, though?  The xwidget code could check whether it's
still displayed and then kill itself it it's not.  It could work along
the lines of the `evaporate' overlay property mechanism, perhaps.

> Which reminds me of another problem (in the Emacs 28 xwidget code as
> well): If an xwidget's buffer is killed, but references to it still
> exist somewhere, crashes can happen, because the GTK resources allocated
> to the xwidget are killed in `kill_buffer_xwidgets' and not during
> garbage collection.

Yes, I've experienced that once already while testing.

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

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: shr using `make-xwidget' incorrectly

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> Lars, you might want to know that `make-xwidget', invoked without a
>> buffer argument, will create an xwidget attached to the current buffer.
>>
>> That xwidget won't be killed until you kill the buffer it's attached to.

> Can't we fix that, though?  The xwidget code could check whether it's
> still displayed and then kill itself it it's not.  It could work along
> the lines of the `evaporate' overlay property mechanism, perhaps.

It's not a bug, but how xwidgets are designed, not unlike asynch
processes that have a buffer.

I find nothing wrong with this design of the xwidget system.

However, randomly killing xwidgets based on whether or not they are
displayed would be undesirable: xwidgets have a lot of state that cannot
be reconstituted through recreating them, and creating them takes a long
time and a lot of memory.

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

Nice, I'll start working on it.  Thanks.

Re: shr using `make-xwidget' incorrectly

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> It's not a bug, but how xwidgets are designed, not unlike asynch
> processes that have a buffer.

I don't see the similarity -- killing the buffer will kill the xwidgets,
while killing the buffer won't kill the process.  On the other hand,
deleting text in a buffer won't make the process become unavailable.

> I find nothing wrong with this design of the xwidget system.
>
> However, randomly killing xwidgets based on whether or not they are
> displayed would be undesirable: xwidgets have a lot of state that cannot
> be reconstituted through recreating them, and creating them takes a long
> time and a lot of memory.

There's nothing random about it.  If the caller has marked the widgets
as being evaporable, then the caller wants those xwidgets to go away if
they're no longer displayed.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: shr using `make-xwidget' incorrectly

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I don't see the similarity -- killing the buffer will kill the xwidgets,
> while killing the buffer won't kill the process.

Sorry, that's not correct (if that's the process-buffer of the process).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: shr using `make-xwidget' incorrectly

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I don't see the similarity -- killing the buffer will kill the xwidgets,
> while killing the buffer won't kill the process.  On the other hand,
> deleting text in a buffer won't make the process become unavailable.

Sorry, I meant only processes with associated buffers.  This is how it
would make sense for xwidgets to behave as well.

BTW, it's impossible to cause an xwidget to become unavailable by
deleting text in a buffer: it'll always be available via
get-buffer-xwidgets, and there could be references to the xwidget in
other places.

At present, the only way to kill an xwidget (and to make it potentially
unavailable) is to kill the buffer it's associated with.

So even if we were to introduce an "evaporable" xwidget type, I think it
would only make sense to have it done as part of garbage collection.

>> However, randomly killing xwidgets based on whether or not they are
>> displayed would be undesirable: xwidgets have a lot of state that cannot
>> be reconstituted through recreating them, and creating them takes a long
>> time and a lot of memory.

> There's nothing random about it.  If the caller has marked the widgets
> as being evaporable, then the caller wants those xwidgets to go away if
> they're no longer displayed.

Making xwidgets "evaporable" would cause a lot of overhead, I think.

For example, WebKitGTK may elect to save or delete cached resources, if
the reference count of the context and settings reaches zero.

(Which will always happen if the widget did not have another widget
passed to it as the `related' parameter to make-xwidget, and may happen
regardless.)

Another solution (that doesn't involve attaching xwidgets to the buffer
currently being rendered in) would be have shr attach xwidgets to a
buffer used only for shr to manage xwidgets, and for shr to "recycle"
the xwidgets each time something is rendered.

Please tell me if I'm missing anything, or you have more comments to
make.

Thanks in advance!

Re: shr using `make-xwidget' incorrectly

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> BTW, it's impossible to cause an xwidget to become unavailable by
> deleting text in a buffer: it'll always be available via
> get-buffer-xwidgets, and there could be references to the xwidget in
> other places.

The same is true for overlays -- you can use `overlays-in' to find all
the overlays in the buffer, even if they aren't otherwise available.  I
think modelling xwidget behaviour on overlays makes a lot of sense from
an UI point of view.

> Making xwidgets "evaporable" would cause a lot of overhead, I think.
>
> For example, WebKitGTK may elect to save or delete cached resources, if
> the reference count of the context and settings reaches zero.
>
> (Which will always happen if the widget did not have another widget
> passed to it as the `related' parameter to make-xwidget, and may happen
> regardless.)

I'm not sure what you mean here.  Are you saying that a widget in a
buffer might just spontaneously disappear?

> Another solution (that doesn't involve attaching xwidgets to the buffer
> currently being rendered in) would be have shr attach xwidgets to a
> buffer used only for shr to manage xwidgets, and for shr to "recycle"
> the xwidgets each time something is rendered.

I don't think the functions that use xwidgets should have to know about
any of this stuff.  Imagine if you'd have to jump through these hoops to
display images?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: shr using `make-xwidget' incorrectly

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> The same is true for overlays -- you can use `overlays-in' to find all
> the overlays in the buffer, even if they aren't otherwise available.  I
> think modelling xwidget behaviour on overlays makes a lot of sense from
> an UI point of view.

Hmm, I will try to think of a reasonable way to implement this.  But
IMO, it should be optional -- the existing behavior makes sense for many
use-cases.

> I'm not sure what you mean here.  Are you saying that a widget in a
> buffer might just spontaneously disappear?

No, that killing a widget might take an unreasonable amount of time and
be surprising to be implemented like evaporating overlays are.

Imagine if the erasure of a few characters took upwards of 1 second,
deleting dozens of on-disk cache files.  That is a situation that might
result from implementing evaporation of xwidgets like with overlays.

> I don't think the functions that use xwidgets should have to know about
> any of this stuff.  Imagine if you'd have to jump through these hoops to
> display images?

Better comparison: imagine if `struct image' was a Lisp object (and not
an internal structure created on the basis of an image spec), held tens
of megabytes of memory, a lot of non-replicable state, an entire
language interpreter, and control over many cache files on-disk.

Then would it still be reasonable to delete `struct image's as part of
some routine pruning operation such as flushing the image cache, or the
erasure of a few characters in a buffer?  I think not.

Thanks.  But I will try my best to find an acceptable solution to this
problem.

Re: shr using `make-xwidget' incorrectly

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

>> The same is true for overlays -- you can use `overlays-in' to find all
>> the overlays in the buffer, even if they aren't otherwise available.  I
>> think modelling xwidget behaviour on overlays makes a lot of sense from
>> an UI point of view.
>
> Hmm, I will try to think of a reasonable way to implement this.  But
> IMO, it should be optional -- the existing behavior makes sense for many
> use-cases.

Yes, indeed.

(And I was wondering why the xwidgets were still playing video when I
hit "back" in the eww history, and landed on a page with a video
again -- but they'd been updating the whole time, even if they'd not
been on screen, I understand now.)

> No, that killing a widget might take an unreasonable amount of time and
> be surprising to be implemented like evaporating overlays are.
>
> Imagine if the erasure of a few characters took upwards of 1 second,
> deleting dozens of on-disk cache files.  That is a situation that might
> result from implementing evaporation of xwidgets like with overlays.

Oh, sure.  I don't expect anybody to be doing exactly that with videos,
for instance, but I think it's a reasonable expectation that a mode that
inserts a video into the buffer shouldn't have to do ... a lot of work
to remove the video, either.  I mean, if that's feasible, it would be
nice.

> Better comparison: imagine if `struct image' was a Lisp object (and not
> an internal structure created on the basis of an image spec), held tens
> of megabytes of memory, a lot of non-replicable state, an entire
> language interpreter, and control over many cache files on-disk.

That describes animated images quite well.  :-)  (OK, the animation
itself is controlled from Lisp land, but there's a bunch of caches and
megabytes of data...)

Hm...  looking at the code in `image-animate-timeout', it may suffer
from some of the same issues...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: shr using `make-xwidget' incorrectly

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Oh, sure.  I don't expect anybody to be doing exactly that with videos,
> for instance, but I think it's a reasonable expectation that a mode that
> inserts a video into the buffer shouldn't have to do ... a lot of work
> to remove the video, either.  I mean, if that's feasible, it would be
> nice.

We could have a separate kind of xwidget for multimedia.  I think there
would be no need to abuse WebKit for this, if that existed.

GStreamer and GTK seem to provide the necessary APIs for dealing with
video streams over the network.

I will look into that, thanks.

Re: shr using `make-xwidget' incorrectly

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> We could have a separate kind of xwidget for multimedia.  I think there
> would be no need to abuse WebKit for this, if that existed.
>
> GStreamer and GTK seem to provide the necessary APIs for dealing with
> video streams over the network.
>
> I will look into that, thanks.

That's be great -- I think most of the "lightweight" incantations of an
xwidget would be for videos and audio, so avoiding firing up Webkit
would be nice.

But the programming UI niceties would still be the same, though (i.e.,
an expectation that we can "make it go away" by deleting it).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: shr using `make-xwidget' incorrectly

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> That's be great -- I think most of the "lightweight" incantations of an
> xwidget would be for videos and audio, so avoiding firing up Webkit
> would be nice.
>
> But the programming UI niceties would still be the same, though (i.e.,
> an expectation that we can "make it go away" by deleting it).

Yes, it would be reasonable to implement such a feature with a separate
video xwidget.  It's just that doing this wth WebKitWebViews makes me
shudder...

Thanks.

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

[Po Lu]

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

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Thu, 11 Nov 2021 14:37:49 +0800
> 
> +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);
> +}

What if the xwidget's buffer was itself killed? should this also test
that situation?

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

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: emacs-devel@gnu.org
>> Date: Thu, 11 Nov 2021 14:37:49 +0800
>> 
>> +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);
>> +}
>
> What if the xwidget's buffer was itself killed? should this also test
> that situation?

When the buffer is killed, all of its xwidgets are killed first, which
means XXWIDGET (object)->buffer will always be nil in that case.

Thanks.

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

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> Subject: [PATCH] Prevent crashes in xwidgets whose buffers have been killed

Looks good to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Looks good to me.

Thanks.  If it looks good to Eli, I'll push.

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

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org
> Date: Thu, 11 Nov 2021 20:25:43 +0800
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > Looks good to me.
> 
> Thanks.  If it looks good to Eli, I'll push.

Yes, LGTM.  Thanks.

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

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: emacs-devel@gnu.org
>> Date: Thu, 11 Nov 2021 20:25:43 +0800
>> 
>> Lars Ingebrigtsen <larsi@gnus.org> writes:
>> 
>> > Looks good to me.
>> 
>> Thanks.  If it looks good to Eli, I'll push.
>
> Yes, LGTM.  Thanks.

Thanks, pushed.

Re: shr using `make-xwidget' incorrectly

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: larsi@gnus.org
> Date: Thu, 11 Nov 2021 09:54:51 +0800
> 
> Which reminds me of another problem (in the Emacs 28 xwidget code as
> well): If an xwidget's buffer is killed, but references to it still
> exist somewhere, crashes can happen, because the GTK resources allocated
> to the xwidget are killed in `kill_buffer_xwidgets' and not during
> garbage collection.
> 
> 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.

Why can't we release the GTK resources when such a buffer is killed?

Re: shr using `make-xwidget' incorrectly

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> Which reminds me of another problem (in the Emacs 28 xwidget code as
>> well): If an xwidget's buffer is killed, but references to it still
>> exist somewhere, crashes can happen, because the GTK resources allocated
>> to the xwidget are killed in `kill_buffer_xwidgets' and not during
>> garbage collection.
>> 
>> 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.

> Why can't we release the GTK resources when such a buffer is killed?

We are releasing the GTK resources when such a buffer is killed.  This
is the problem here: code that uses `CHECK_XWIDGET' won't catch the case
where the GTK resources are released, but there are still references to
the xwidget, which causes use-after-frees when trying to redisplay the
xwidget, or when calling, say, `xwidget-webkit-goto-uri' on the xwidget.

The comment in kill_buffer_xwidgets that says:

      /* TODO free the GTK things in xw.  */

Is probably outdated and likely should be removed.  But I am still
investigating through the history of that file to make sure of that, so
please bear with me for a while.

Thanks.

Re: shr using `make-xwidget' incorrectly

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: larsi@gnus.org,  emacs-devel@gnu.org
> Date: Thu, 11 Nov 2021 15:05:19 +0800
> 
> Eli Zaretskii <eliz@gnu.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.
> 
> > Why can't we release the GTK resources when such a buffer is killed?
> 
> We are releasing the GTK resources when such a buffer is killed.  This
> is the problem here: code that uses `CHECK_XWIDGET' won't catch the case
> where the GTK resources are released, but there are still references to
> the xwidget, which causes use-after-frees when trying to redisplay the
> xwidget, or when calling, say, `xwidget-webkit-goto-uri' on the xwidget.

Isn't there some indication in the xwidget that its GTK resources were
freed?  If not, can we add such an indication and set it when we free
those resources?

Re: shr using `make-xwidget' incorrectly

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> We are releasing the GTK resources when such a buffer is killed.  This
>> is the problem here: code that uses `CHECK_XWIDGET' won't catch the case
>> where the GTK resources are released, but there are still references to
>> the xwidget, which causes use-after-frees when trying to redisplay the
>> xwidget, or when calling, say, `xwidget-webkit-goto-uri' on the xwidget.

> Isn't there some indication in the xwidget that its GTK resources were
> freed?  If not, can we add such an indication and set it when we free
> those resources?

There isn't, but I added one in the patch I sent Lars.