bug#24041: 25.1.50; xwidget + -nw mode gives segfault

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Shane Hansen]

Segfault when attempting to open xwidget from terminal emacs (-nw). I
expect this not to work, but I also don't expect a segfault.

Steps to reproduce:

1. Build emacs
2. run emacs via emacs -nw
3. Type M-x xwidget-webkit-browse-url
4. http://www.google.com

Expected behaviour:
error message due to lack of x windowing system

Actual behaviour:
segfault


begin bt:
#0 0x00007ffff6df3349 in gtk_style_context_get_background_color ()
from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#1 0x00007ffff6e27746 in ?? () from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#2 0x00007ffff6e2bc00 in ?? () from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#3 0x00007ffff6e2bdc8 in ?? () from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#4 0x00007ffff6e352f3 in ?? () from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
#5 0x00007ffff5bf2789 in ?? () from
/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#6 0x00007ffff5bf410d in g_object_newv () from
/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#7 0x00007ffff5bf48bc in g_object_new () from
/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#8 0x00007ffff22a3aa1 in ?? () from
/usr/lib/x86_64-linux-gnu/libwebkitgtk-3.0.so.0
#9 0x00007ffff218e272 in ?? () from
/usr/lib/x86_64-linux-gnu/libwebkitgtk-3.0.so.0
#10 0x00007ffff21bb073 in ?? () from
/usr/lib/x86_64-linux-gnu/libwebkitgtk-3.0.so.0
#11 0x00007ffff5c0de3b in g_type_create_instance () from
/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#12 0x00007ffff5bf2355 in ?? () from
/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#13 0x00007ffff5bf410d in g_object_newv () from
/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#14 0x00007ffff5bf48bc in g_object_new () from
/usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0
#15 0x00007ffff21b72c2 in webkit_web_view_new () from
/usr/lib/x86_64-linux-gnu/libwebkitgtk-3.0.so.0
#16 0x0000000000678951 in Fmake_xwidget (type=50400, title=34209780,
width=4002, height=4002, arguments=0, buffer=0) at xwidget.c:139
#17 0x00000000005ffe63 in Ffuncall (nargs=6, args=0x7fffffffacc8) at eval.c:2722
#18 0x000000000064542e in exec_byte_code (bytestr=37473540,
vector=38110277, maxdepth=58, args_template=6166, nargs=5,
args=0x7fffffffb280) at bytecode.c:802
#19 0x00000000006005af in funcall_lambda (fun=38110325, nargs=5,
arg_vector=0x7fffffffb258) at eval.c:2863
#20 0x00000000005fff9f in Ffuncall (nargs=6, args=0x7fffffffb250) at eval.c:2750
#21 0x000000000064542e in exec_byte_code (bytestr=34209940,
vector=38102717, maxdepth=38, args_template=1030, nargs=1,
args=0x7fffffffb7c0) at bytecode.c:802
#22 0x00000000006005af in funcall_lambda (fun=38102861, nargs=1,
arg_vector=0x7fffffffb7b8) at eval.c:2863
#23 0x00000000005fff9f in Ffuncall (nargs=2, args=0x7fffffffb7b0) at eval.c:2750
#24 0x000000000064542e in exec_byte_code (bytestr=34195700,
vector=38102909, maxdepth=18, args_template=1030, nargs=1,
args=0x7fffffffbd18) at bytecode.c:802
#25 0x00000000006005af in funcall_lambda (fun=38102941, nargs=1,
arg_vector=0x7fffffffbd10) at eval.c:2863
#26 0x00000000005fff9f in Ffuncall (nargs=2, args=0x7fffffffbd08) at eval.c:2750
#27 0x000000000064542e in exec_byte_code (bytestr=35946404,
vector=38099989, maxdepth=18, args_template=2054, nargs=2,
args=0x7fffffffc350) at bytecode.c:802
#28 0x00000000006005af in funcall_lambda (fun=38100085, nargs=2,
arg_vector=0x7fffffffc340) at eval.c:2863
#29 0x00000000005fff9f in Ffuncall (nargs=3, args=0x7fffffffc338) at eval.c:2750
#30 0x00000000005f7c58 in Ffuncall_interactively (nargs=3,
args=0x7fffffffc338) at callint.c:252
#31 0x00000000005ffbdc in Ffuncall (nargs=4, args=0x7fffffffc330) at eval.c:2681
#32 0x00000000005ff142 in Fapply (nargs=3, args=0x7fffffffc560) at eval.c:2329
#33 0x00000000005f80d3 in Fcall_interactively (function=4303704,
record_flag=4857400, keys=13531957) at callint.c:389
#34 0x00000000005ffd67 in Ffuncall (nargs=4, args=0x7fffffffc6b8) at eval.c:2708
#35 0x000000000064542e in exec_byte_code (bytestr=10581756,
vector=10581789, maxdepth=54, args_template=4102, nargs=2,
args=0x7fffffffcc58) at bytecode.c:802
#36 0x00000000006005af in funcall_lambda (fun=10581709, nargs=2,
arg_vector=0x7fffffffcc48) at eval.c:2863
#37 0x00000000005fff9f in Ffuncall (nargs=3, args=0x7fffffffcc40) at eval.c:2750
#38 0x000000000064542e in exec_byte_code (bytestr=10580948,
vector=10580981, maxdepth=62, args_template=3078, nargs=3,
args=0x7fffffffd2c8) at bytecode.c:802
#39 0x00000000006005af in funcall_lambda (fun=10580893, nargs=3,
arg_vector=0x7fffffffd2b0) at eval.c:2863
#40 0x00000000005fff9f in Ffuncall (nargs=4, args=0x7fffffffd2a8) at eval.c:2750
#41 0x00000000005f7c58 in Ffuncall_interactively (nargs=4,
args=0x7fffffffd2a8) at callint.c:252
#42 0x00000000005ffbdc in Ffuncall (nargs=5, args=0x7fffffffd2a0) at eval.c:2681
#43 0x00000000005ff142 in Fapply (nargs=3, args=0x7fffffffd4e0) at eval.c:2329
#44 0x00000000005f80d3 in Fcall_interactively (function=590280,
record_flag=0, keys=13531957) at callint.c:389
#45 0x00000000005ffd67 in Ffuncall (nargs=4, args=0x7fffffffd638) at eval.c:2708
#46 0x000000000064542e in exec_byte_code (bytestr=10581756,
vector=10581789, maxdepth=54, args_template=4102, nargs=1,
args=0x7fffffffdbb0) at bytecode.c:802
#47 0x00000000006005af in funcall_lambda (fun=10581709, nargs=1,
arg_vector=0x7fffffffdba8) at eval.c:2863
#48 0x00000000005fff9f in Ffuncall (nargs=2, args=0x7fffffffdba0) at eval.c:2750
#49 0x00000000005ff6e0 in call1 (fn=15168, arg1=590280) at eval.c:2560
#50 0x000000000055f10c in command_loop_1 () at keyboard.c:1484
#51 0x00000000005fc874 in internal_condition_case (bfun=0x55e91d
<command_loop_1>, handlers=19488, hfun=0x55e0e1 <cmd_error>) at
eval.c:1310
#52 0x000000000055e625 in command_loop_2 (ignore=0) at keyboard.c:1112
#53 0x00000000005fc1bb in internal_catch (tag=46368, func=0x55e5fc
<command_loop_2>, arg=0) at eval.c:1075
#54 0x000000000055e5c5 in command_loop () at keyboard.c:1091
#55 0x000000000055dcb6 in recursive_edit_1 () at keyboard.c:697
#56 0x000000000055de43 in Frecursive_edit () at keyboard.c:768
#57 0x000000000055bba7 in main (argc=2, argv=0x7fffffffe058) at emacs.c:1658

end bt

In GNU Emacs 25.1.50.2 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8)
of 2016-07-20 built on saidin
Repository revision: 30b3a842ec87d27cfe003b6d4323689d48b3fcd2
Windowing system distributor 'The X.Org Foundation', version 11.0.11501000
System Description: Linux Mint 17.2 Rafaela

Configured using:
'configure --with-xwidgets --with-x-toolkit=gtk3 'CFLAGS= -g''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XWIDGETS

Important settings:
value of $LANG: en_US.UTF-8
locale-coding-system: utf-8-unix

Major mode: Fundamental

Minor modes in effect:
shell-dirtrack-mode: t
global-auto-complete-mode: t
show-paren-mode: t
save-place-mode: t
tooltip-mode: t
global-eldoc-mode: t
electric-indent-mode: t
mouse-wheel-mode: t
file-name-shadow-mode: t
global-font-lock-mode: t
blink-cursor-mode: t
auto-composition-mode: t
auto-encryption-mode: t
auto-compression-mode: t
buffer-read-only: t
column-number-mode: t
line-number-mode: t
transient-mark-mode: t


Features:
(shadow sort mail-extr emacsbug message puny dired dired-loaddefs rfc822
mml mml-sec epa derived epg gnus-util rmail rmail-loaddefs mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
warnings ox-md ox-confluence ox-latex ox-icalendar ox-html ox-ascii
ox-publish ox org-element avl-tree org-clock windmove ob-dot ob-sh shell
ob-go ob-python org org-macro org-footnote org-pcomplete pcomplete
org-list org-faces org-entities noutline outline easy-mmode org-version
ob-emacs-lisp ob ob-tangle ob-ref ob-lob ob-table ob-exp org-src ob-keys
ob-comint ob-core ob-eval org-compat org-macs org-loaddefs format-spec
cal-menu calendar cal-loaddefs go-oracle go-autocomplete s ucs-normalize
flycheck find-func rx subr-x dash auto-complete-config auto-complete
popup flymake-cursor flymake compile comint ansi-color go-mode url
url-proxy url-privacy url-expand url-methods url-history url-cookie
url-domsuf url-util mailcap find-file ffap thingatpt etags xref project
ring cl async finder-inf async-autoloads better-defaults-autoloads paren
edmacro kmacro saveplace ido cider-autoloads clojure-mode-autoloads
docker-tramp-autoloads dockerfile-mode-autoloads flycheck-autoloads
flymake-cursor-autoloads advice gh-md-autoloads gntp-autoloads
go-autocomplete-autoloads auto-complete-autoloads go-mode-autoloads
graphviz-dot-mode-autoloads htmlize-autoloads log4e-autoloads
lua-mode-autoloads markdown-mode-autoloads ob-go-autoloads
ob-lua-autoloads org-jira-autoloads org-plus-contrib-autoloads
ox-reveal-autoloads org-autoloads popup-autoloads projectile-autoloads
pkg-info-autoloads epl-autoloads dash-autoloads queue-autoloads
s-autoloads salt-mode-autoloads mmm-jinja2-autoloads info
mmm-mode-autoloads seq-autoloads speck-autoloads spinner-autoloads
yaml-mode-autoloads package epg-config url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache url-vars seq byte-opt gv bytecomp byte-compile cl-extra
help-mode easymenu cconv cl-loaddefs pcase cl-lib time-date mule-util
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list newcomment elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow timer select scroll-bar
mouse jit-lock font-lock syntax facemenu font-core term/tty-colors frame
cl-generic cham georgian utf-8-lang misc-lang vietnamese tibetan thai
tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek romanian
slovak czech european ethiopic indian cyrillic chinese charscript
case-table epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote dbusbind inotify
dynamic-setting system-font-setting font-render-setting xwidget-internal
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 403830 9738)
(symbols 48 38346 0)
(miscs 40 140 158)
(strings 32 85566 14409)
(string-bytes 1 2582019)
(vectors 16 56565)
(vector-slots 8 927530 4481)
(floats 8 362 30)
(intervals 56 339 0)
(buffers 976 14)
(heap 1024 44445 4484))

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Robert Cochran]

First, apologies for having left this alone this long. I meant to get to
it earlier.

Anyways, so the fix here is conceptually pretty simple: have a check in
place to ensure (display-graphic-p) returns non-nil before doing any
xwidgets stuff. Easy enough.

What I want to solicit feedback on before I write a patch is this: who
should be responsible for this check? Should the function provider (the
xwidget C 'library') check for the proper support? Or should that be
left to the user (the Lisp that calls the xwidget functions, in this
case `xwidget-webkit-browse-url`)?

Personal opinion is that xwidget should check: that way a Lisp caller
can't forget to do so. But I'm largely unaware of the conventions on how
to handle this, so I want to make sure I'm doing the correct thing
before I make a style mistake.

Thanks,
-- 
~Robert Cochran

GPG Fingerprint - E778 2DD4 FEA6 6A68 6F26  AD2D E5C3 EB36 4886 8871

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Eli Zaretskii]

> From: Robert Cochran <robert-emacs@cochranmail.com>
> Date: Fri, 19 Aug 2016 11:36:30 -0700
> Cc: 24041@debbugs.gnu.org
> 
> Anyways, so the fix here is conceptually pretty simple: have a check in
> place to ensure (display-graphic-p) returns non-nil before doing any
> xwidgets stuff. Easy enough.

display-graphic-p accepts argument, so if a function displays on
another frame or display, it should be passed that frame or display.

> What I want to solicit feedback on before I write a patch is this: who
> should be responsible for this check? Should the function provider (the
> xwidget C 'library') check for the proper support? Or should that be
> left to the user (the Lisp that calls the xwidget functions, in this
> case `xwidget-webkit-browse-url`)?

IMO, the check should be on the C level where the xwidget primitives
are implemented.  One such place is make-xwidget; maybe there are
more.  (The proper way of testing this on the C level is not by
calling display-graphic-p, but rather with check_x_display_info.)

Thanks.

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Robert Cochran]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Cochran <robert-emacs@cochranmail.com>
>> Date: Fri, 19 Aug 2016 11:36:30 -0700
>> Cc: 24041@debbugs.gnu.org
>> 
>> Anyways, so the fix here is conceptually pretty simple: have a check in
>> place to ensure (display-graphic-p) returns non-nil before doing any
>> xwidgets stuff. Easy enough.
>
> display-graphic-p accepts argument, so if a function displays on
> another frame or display, it should be passed that frame or display.

I guess I don't quite understand what you're trying to say here. In the
particular case of `xwidget-webkit-browse-url`, the current frame
is reused. I don't think it would be a particularly good idea to
automatically redirect widget requests to a graphical frame. (Is that
even possible? Start a graphical frame from a tty Emacs?)

Again, I'm having trouble understanding what you mean. Apologies about
my confusion.

>> What I want to solicit feedback on before I write a patch is this: who
>> should be responsible for this check? Should the function provider (the
>> xwidget C 'library') check for the proper support? Or should that be
>> left to the user (the Lisp that calls the xwidget functions, in this
>> case `xwidget-webkit-browse-url`)?
>
> IMO, the check should be on the C level where the xwidget primitives
> are implemented.  One such place is make-xwidget; maybe there are
> more.  (The proper way of testing this on the C level is not by
> calling display-graphic-p, but rather with check_x_display_info.)

That's what my feeling was too. I'll go ahead and do it on the C level
then.

> Thanks.

No problem.

-- 
~Robert Cochran

GPG Fingerprint - E778 2DD4 FEA6 6A68 6F26  AD2D E5C3 EB36 4886 8871

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Eli Zaretskii]

> From: Robert Cochran <robert-emacs@cochranmail.com>
> Cc: Robert Cochran <robert-emacs@cochranmail.com>,  shanemhansen@gmail.com,  24041@debbugs.gnu.org
> Date: Sat, 20 Aug 2016 14:33:58 -0700
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Robert Cochran <robert-emacs@cochranmail.com>
> >> Date: Fri, 19 Aug 2016 11:36:30 -0700
> >> Cc: 24041@debbugs.gnu.org
> >> 
> >> Anyways, so the fix here is conceptually pretty simple: have a check in
> >> place to ensure (display-graphic-p) returns non-nil before doing any
> >> xwidgets stuff. Easy enough.
> >
> > display-graphic-p accepts argument, so if a function displays on
> > another frame or display, it should be passed that frame or display.
> 
> I guess I don't quite understand what you're trying to say here.

I'm saying that display-graphic-p should be told the frame for which
you are asking the question.

> (Is that even possible? Start a graphical frame from a tty Emacs?)

Of course, it is.  emacsclient can do that.  Assuming Emacs was built
with GUI support, of course.  We have this feature for several years
now.

> > IMO, the check should be on the C level where the xwidget primitives
> > are implemented.  One such place is make-xwidget; maybe there are
> > more.  (The proper way of testing this on the C level is not by
> > calling display-graphic-p, but rather with check_x_display_info.)
> 
> That's what my feeling was too. I'll go ahead and do it on the C level
> then.

Thanks.

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Robert Cochran]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> (Is that even possible? Start a graphical frame from a tty Emacs?)
>
> Of course, it is.  emacsclient can do that.  Assuming Emacs was built
> with GUI support, of course.  We have this feature for several years
> now.
>

Indeed it can. Never tried it before, and kinda assumed it wouldn't
work. Silly me.

Anyways, I have a patch, below, that's my first stab at solving the
problem. All it does is call `check_x_display_info` with the current
frame and allows any resulting error signals to propagate back up.

Probably not the most elegant solution, but I'm not entirely clear what
can and can't be done from within the Emacs C core. Suggestions are very
welcome.

-----


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Patch to see if the frame is an X frame before creating xwidget --]
[-- Type: text/x-patch, Size: 1328 bytes --]

From 82532f657f7acc824745406e8917a1a4f49723d9 Mon Sep 17 00:00:00 2001
From: Robert Cochran <robert-git@cochranmail.com>
Date: Sun, 21 Aug 2016 19:01:14 -0700
Subject: [PATCH] Signal an error when a TTY frame tries to create an xwidget

* src/xwidget.c (make-xwidget): signal an error if current frame is a
  TTY frame.

Trying to use a TTY frame to create an xwidget can cause a
segfault. Check before hand that the frame is an X frame via
check_x_display_info, which will signal an error if it is not. This
fixes Bug#24041.
---
 src/xwidget.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/xwidget.c b/src/xwidget.c
index f5f4da0..0edfacc 100644
--- a/src/xwidget.c
+++ b/src/xwidget.c
@@ -99,8 +99,15 @@ Returns the newly constructed xwidget, or nil if construction fails.  */)
   CHECK_NATNUM (width);
   CHECK_NATNUM (height);
 
-  struct xwidget *xw = allocate_xwidget ();
+  struct xwidget *xw;
   Lisp_Object val;
+
+  /* Ensure that the current frame is an X frame before we try
+     creating any xwidgets. If it isn't, check_x_display_info will
+     signal an error.  */
+  check_x_display_info (Fselected_frame ());
+
+  xw = allocate_xwidget ();
   xw->type = type;
   xw->title = title;
   xw->buffer = NILP (buffer) ? Fcurrent_buffer () : Fget_buffer_create (buffer);
-- 
2.7.4


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

-----

HTH,
-- 
~Robert Cochran

GPG Fingerprint - E778 2DD4 FEA6 6A68 6F26  AD2D E5C3 EB36 4886 8871

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Eli Zaretskii]

> From: Robert Cochran <robert-emacs@cochranmail.com>
> Cc: Robert Cochran <robert-emacs@cochranmail.com>,  shanemhansen@gmail.com,  24041@debbugs.gnu.org
> Date: Sun, 21 Aug 2016 19:12:41 -0700
> 
> Anyways, I have a patch, below, that's my first stab at solving the
> problem. All it does is call `check_x_display_info` with the current
> frame and allows any resulting error signals to propagate back up.
> 
> Probably not the most elegant solution, but I'm not entirely clear what
> can and can't be done from within the Emacs C core. Suggestions are very
> welcome.

My only comment is that you could call check_x_display_info with Qnil
as its argument.

Otherwise, LGTM, thanks.  And I see nothing inelegant in this patch.

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Robert Cochran]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Cochran <robert-emacs@cochranmail.com>
>> Cc: Robert Cochran <robert-emacs@cochranmail.com>,  shanemhansen@gmail.com,  24041@debbugs.gnu.org
>> Date: Sun, 21 Aug 2016 19:12:41 -0700
>> 
>> Anyways, I have a patch, below, that's my first stab at solving the
>> problem. All it does is call `check_x_display_info` with the current
>> frame and allows any resulting error signals to propagate back up.
>> 
>> Probably not the most elegant solution, but I'm not entirely clear what
>> can and can't be done from within the Emacs C core. Suggestions are very
>> welcome.
>
> My only comment is that you could call check_x_display_info with Qnil
> as its argument.

I did think about that. But then it arguably does the wrong thing:
`check_x_display_info` with `Qnil` only signals an error when there have
never been X windows, eg, opening and closing an X window satisfies the
check from then on. It no longer crashes in that instance, but I
personally don't think that's the right behavior; if my starting frame
isn't capable of displaying an xwidget, say so! Hence checking with the
current frame.

That, of course, is just my opinion.

> Otherwise, LGTM, thanks.  And I see nothing inelegant in this patch.

Thanks for your reassurance! My one gripe about this patch is that I
didn't figure out how to kill the buffer after xwidget creation failure
(leaving it seems rather ugly IMO), but I just now realized what I can
do. As long as it's not considered wrong to kill a mode's buffer on
error, would you also consider this patch to go along with it?

-----


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Kill webkit browser window if error occurs --]
[-- Type: text/x-patch, Size: 1499 bytes --]

From e58ed09121969febde5bfc2206c14f4a7806c323 Mon Sep 17 00:00:00 2001
From: Robert Cochran <robert-git@cochranmail.com>
Date: Mon, 22 Aug 2016 11:21:01 -0700
Subject: [PATCH] Kill the webkit browser buffer if an error occurs

* lisp/xwidget.el (xwidget-webkit-new-session): kill webkit buffer on
  error
---
 lisp/xwidget.el | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/lisp/xwidget.el b/lisp/xwidget.el
index 7a0ca8b..e925c9a 100644
--- a/lisp/xwidget.el
+++ b/lisp/xwidget.el
@@ -427,11 +427,16 @@ xwidget-webkit-new-session
        xw)
     (setq xwidget-webkit-last-session-buffer (switch-to-buffer
                                               (get-buffer-create bufname)))
-    (insert " 'a' adjusts the xwidget size.")
-    (setq xw (xwidget-insert 1 'webkit  bufname 1000 1000))
-    (xwidget-put xw 'callback 'xwidget-webkit-callback)
-    (xwidget-webkit-mode)
-    (xwidget-webkit-goto-uri (xwidget-webkit-last-session) url)))
+    (condition-case err
+        (progn
+          (insert " 'a' adjusts the xwidget size.")
+          (setq xw (xwidget-insert 1 'webkit  bufname 1000 1000))
+          (xwidget-put xw 'callback 'xwidget-webkit-callback)
+          (xwidget-webkit-mode)
+          (xwidget-webkit-goto-uri (xwidget-webkit-last-session) url))
+      ;; On error, remove webkit buffer and resignal
+      (error (kill-buffer bufname)
+             (signal (car err) (cdr err))))))
 
 
 (defun xwidget-webkit-goto-url (url)
-- 
2.7.4


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

-----

Otherwise, with or without the new patch, I have no issues if you have
no issues.

Thanks,
-- 
~Robert Cochran

GPG Fingerprint - E778 2DD4 FEA6 6A68 6F26  AD2D E5C3 EB36 4886 8871

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Eli Zaretskii]

> From: Robert Cochran <robert-emacs@cochranmail.com>
> Cc: Robert Cochran <robert-emacs@cochranmail.com>,  shanemhansen@gmail.com,  24041@debbugs.gnu.org
> Date: Mon, 22 Aug 2016 11:30:15 -0700
> 
> > My only comment is that you could call check_x_display_info with Qnil
> > as its argument.
> 
> I did think about that. But then it arguably does the wrong thing:
> `check_x_display_info` with `Qnil` only signals an error when there have
> never been X windows, eg, opening and closing an X window satisfies the
> check from then on. It no longer crashes in that instance, but I
> personally don't think that's the right behavior; if my starting frame
> isn't capable of displaying an xwidget, say so! Hence checking with the
> current frame.

Joakim, is it certain that the xwidget will always be shown in the
frame that is the selected one at the time make-xwidget is called?

> Thanks for your reassurance! My one gripe about this patch is that I
> didn't figure out how to kill the buffer after xwidget creation failure
> (leaving it seems rather ugly IMO), but I just now realized what I can
> do. As long as it's not considered wrong to kill a mode's buffer on
> error, would you also consider this patch to go along with it?

I'm not sure if this is TRT.  I'd rather erase-buffer at the beginning
of xwidget-webkit-new-session, and leave the buffer alone if we signal
an error.  The buffer might have contents that the user will hate
losing, for diagnostic purposes if nothing else.

Joakim, WDYT?

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[joakim]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Cochran <robert-emacs@cochranmail.com>
>> Cc: Robert Cochran <robert-emacs@cochranmail.com>,  shanemhansen@gmail.com,  24041@debbugs.gnu.org
>> Date: Mon, 22 Aug 2016 11:30:15 -0700
>> 
>> > My only comment is that you could call check_x_display_info with Qnil
>> > as its argument.
>> 
>> I did think about that. But then it arguably does the wrong thing:
>> `check_x_display_info` with `Qnil` only signals an error when there have
>> never been X windows, eg, opening and closing an X window satisfies the
>> check from then on. It no longer crashes in that instance, but I
>> personally don't think that's the right behavior; if my starting frame
>> isn't capable of displaying an xwidget, say so! Hence checking with the
>> current frame.
>
> Joakim, is it certain that the xwidget will always be shown in the
> frame that is the selected one at the time make-xwidget is called?

make-xwidget doesnt actualy show the widget.

There is code like this in xwidget-insert:
    (put-text-property (point) (+ 1 (point))
                       'display (list 'xwidget ':xwidget id))


>
>> Thanks for your reassurance! My one gripe about this patch is that I
>> didn't figure out how to kill the buffer after xwidget creation failure
>> (leaving it seems rather ugly IMO), but I just now realized what I can
>> do. As long as it's not considered wrong to kill a mode's buffer on
>> error, would you also consider this patch to go along with it?
>
> I'm not sure if this is TRT.  I'd rather erase-buffer at the beginning
> of xwidget-webkit-new-session, and leave the buffer alone if we signal
> an error.  The buffer might have contents that the user will hate
> losing, for diagnostic purposes if nothing else.
>
> Joakim, WDYT?

I tried to model the code originaly after what the various emacs image
modes did, mostly along the principle of least surprise. I think emacs
normally works like Eli describes above, so I'd go with that yes.



-- 
Joakim Verona

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Robert Cochran]

joakim@verona.se writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Robert Cochran <robert-emacs@cochranmail.com>
>>> Cc: Robert Cochran <robert-emacs@cochranmail.com>,  shanemhansen@gmail.com,  24041@debbugs.gnu.org
>>> Date: Mon, 22 Aug 2016 11:30:15 -0700
>>> 
>>> > My only comment is that you could call check_x_display_info with Qnil
>>> > as its argument.
>>> 
>>> I did think about that. But then it arguably does the wrong thing:
>>> `check_x_display_info` with `Qnil` only signals an error when there have
>>> never been X windows, eg, opening and closing an X window satisfies the
>>> check from then on. It no longer crashes in that instance, but I
>>> personally don't think that's the right behavior; if my starting frame
>>> isn't capable of displaying an xwidget, say so! Hence checking with the
>>> current frame.
>>
>> Joakim, is it certain that the xwidget will always be shown in the
>> frame that is the selected one at the time make-xwidget is called?
>
> make-xwidget doesnt actualy show the widget.
>
> There is code like this in xwidget-insert:
>     (put-text-property (point) (+ 1 (point))
>                        'display (list 'xwidget ':xwidget id))

I'm having trouble figuring out a workable compromise on this. While
calling code can just be aware that the frame must be graphical, it
probably isn't the optimal solution.

I suppose I'm at a loss for a situation where you are using a tty and
yet want a mode with xwidgets, although I have things set up in such a
way that I never open a tty emacsclient unless I've logged in remotely
or have chosen to forgo an X session (neither of which happen often in
practice); I have fairly convenient ways across DE/WMs to open a
graphical emacsclient.

>>> Thanks for your reassurance! My one gripe about this patch is that I
>>> didn't figure out how to kill the buffer after xwidget creation failure
>>> (leaving it seems rather ugly IMO), but I just now realized what I can
>>> do. As long as it's not considered wrong to kill a mode's buffer on
>>> error, would you also consider this patch to go along with it?
>>
>> I'm not sure if this is TRT.  I'd rather erase-buffer at the beginning
>> of xwidget-webkit-new-session, and leave the buffer alone if we signal
>> an error.  The buffer might have contents that the user will hate
>> losing, for diagnostic purposes if nothing else.
>>
>> Joakim, WDYT?
>
> I tried to model the code originaly after what the various emacs image
> modes did, mostly along the principle of least surprise. I think emacs
> normally works like Eli describes above, so I'd go with that yes.

Fair enough. Again, merely my opinion that leaving dead buffers is
ugly. If I'm deviating from Emacs standard practice in killing the
buffer, then we're done here. Patch doesn't go in, life continues on.

-- 
~Robert Cochran

GPG Fingerprint - E778 2DD4 FEA6 6A68 6F26  AD2D E5C3 EB36 4886 8871

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Eli Zaretskii]

> From: Robert Cochran <robert-emacs@cochranmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Robert Cochran <robert-emacs@cochranmail.com>,  shanemhansen@gmail.com,  24041@debbugs.gnu.org
> Date: Mon, 22 Aug 2016 13:28:51 -0700
> 
> I suppose I'm at a loss for a situation where you are using a tty and
> yet want a mode with xwidgets, although I have things set up in such a
> way that I never open a tty emacsclient unless I've logged in remotely
> or have chosen to forgo an X session (neither of which happen often in
> practice); I have fairly convenient ways across DE/WMs to open a
> graphical emacsclient.

Some code might prepare the xwidget first, and only later show it in a
frame it creates for that.  It's a valid use case, so I don't think we
should preclude it if not necessary.

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Lars Ingebrigtsen]

Shane Hansen <shanemhansen@gmail.com> writes:

> Segfault when attempting to open xwidget from terminal emacs (-nw). I
> expect this not to work, but I also don't expect a segfault.
>
> Steps to reproduce:
>
> 1. Build emacs
> 2. run emacs via emacs -nw
> 3. Type M-x xwidget-webkit-browse-url
> 4. http://www.google.com
>
> Expected behaviour:
> error message due to lack of x windowing system
>
> Actual behaviour:
> segfault
>
> begin bt:
> #0 0x00007ffff6df3349 in gtk_style_context_get_background_color ()
> from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
> #1 0x00007ffff6e27746 in ?? () from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0

If I try this in Emacs 27, I just get the following error message:

xwidget-insert: make-xwidget: GTK has not been initialized

So I assume that this has been fixed in the years since this was
reported, and am closing this bug report.  If this problem is still
present, please reopen.

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

bug#24041: 25.1.50; xwidget + -nw mode gives segfault

[Robert Pluim]

forcemerge 33294 24041
quit

>>>>> On Wed, 28 Aug 2019 22:04:24 +0200, Lars Ingebrigtsen <larsi@gnus.org> said:
    >> begin bt:
    >> #0 0x00007ffff6df3349 in gtk_style_context_get_background_color ()
    >> from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0
    >> #1 0x00007ffff6e27746 in ?? () from /usr/lib/x86_64-linux-gnu/libgtk-3.so.0

    Lars> If I try this in Emacs 27, I just get the following error message:

    Lars> xwidget-insert: make-xwidget: GTK has not been initialized

    Lars> So I assume that this has been fixed in the years since this was
    Lars> reported, and am closing this bug report.  If this problem is still
    Lars> present, please reopen.

Yep, by a291f62428, which is in 26.2

Robert