Subject: bug#31545: closed (Re: bug#31545: xwidget-webkit-execute-script does not protect script against GC)

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 21 May 2018 20:55:02 +0000

Your bug report

#31545: xwidget-webkit-execute-script does not protect script against GC

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
31545: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=31545
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#31545: xwidget-webkit-execute-script does not protect script against GC
From: Andreas Schwab <[email-masked]>
To: [email-masked]
Date: Mon, 21 May 2018 22:54:49 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

I have pushed this patch to fix the GC problem in
xwidget-webkit-execute-script.

Andreas.

* src/xwidget.h (struct xwidget): Add script_callbacks.
* src/xwidget.c (save_script_callback): New function.
(Fxwidget_webkit_execute_script): Use it.  Encode script
before passing to execution engine.  Always use a callback.
(webkit_javascript_finished_cb): Deallocate script.
(kill_buffer_xwidgets): Deallocate remaining scripts.
(Fxwidget_webkit_zoom): Doc fix.
(Fxwidget_resize): Doc fix.
---
 src/xwidget.c | 99 +++++++++++++++++++++++++++++++++++++--------------
 src/xwidget.h |  3 ++
 2 files changed, 75 insertions(+), 27 deletions(-)

diff --git a/src/xwidget.c b/src/xwidget.c
index 95fa5f19c4..c4a3b1990d 100644
--- a/src/xwidget.c
+++ b/src/xwidget.c
@@ -362,7 +362,7 @@ webkit_js_to_lisp (JSContextRef context, JSValueRef value)
 static void
 webkit_javascript_finished_cb (GObject      *webview,
                                GAsyncResult *result,
-                               gpointer      lisp_callback)
+                               gpointer      arg)
 {
     WebKitJavascriptResult *js_result;
     JSValueRef value;
@@ -370,6 +370,11 @@ webkit_javascript_finished_cb (GObject      *webview,
     GError *error = NULL;
     struct xwidget *xw = g_object_get_data (G_OBJECT (webview),
                                             XG_XWIDGET);
+    ptrdiff_t script_idx = (ptrdiff_t) arg;
+    Lisp_Object script_callback = AREF (xw->script_callbacks, script_idx);
+    ASET (xw->script_callbacks, script_idx, Qnil);
+    if (!NILP (script_callback))
+      xfree (XSAVE_POINTER (XCAR (script_callback), 0));
 
     js_result = webkit_web_view_run_javascript_finish
       (WEBKIT_WEB_VIEW (webview), result, &error);
@@ -381,18 +386,19 @@ webkit_javascript_finished_cb (GObject      *webview,
         return;
       }
 
-    context = webkit_javascript_result_get_global_context (js_result);
-    value = webkit_javascript_result_get_value (js_result);
-    Lisp_Object lisp_value = webkit_js_to_lisp (context, value);
-    webkit_javascript_result_unref (js_result);
+    if (!NILP (script_callback) && !NILP (XCDR (script_callback)))
+      {
+	context = webkit_javascript_result_get_global_context (js_result);
+	value = webkit_javascript_result_get_value (js_result);
+	Lisp_Object lisp_value = webkit_js_to_lisp (context, value);
+
+	/* Register an xwidget event here, which then runs the callback.
+	   This ensures that the callback runs in sync with the Emacs
+	   event loop.  */
+	store_xwidget_js_callback_event (xw, XCDR (script_callback), lisp_value);
+      }
 
-    /* Register an xwidget event here, which then runs the callback.
-       This ensures that the callback runs in sync with the Emacs
-       event loop.  */
-    /* FIXME: This might lead to disaster if LISP_CALLBACK's object
-       was garbage collected before now.  See the FIXME in
-       Fxwidget_webkit_execute_script.  */
-    store_xwidget_js_callback_event (xw, XPL (lisp_callback), lisp_value);
+    webkit_javascript_result_unref (js_result);
 }

@@ -684,8 +690,7 @@ DEFUN ("xwidget-webkit-goto-uri",
 DEFUN ("xwidget-webkit-zoom",
        Fxwidget_webkit_zoom, Sxwidget_webkit_zoom,
        2, 2, 0,
-       doc: /* Change the zoom factor of the xwidget webkit instance
-referenced by XWIDGET.  */)
+       doc: /* Change the zoom factor of the xwidget webkit instance referenced by XWIDGET.  */)
   (Lisp_Object xwidget, Lisp_Object factor)
 {
   WEBKIT_FN_INIT ();
@@ -700,12 +705,46 @@ referenced by XWIDGET.  */)
   return Qnil;
 }
 
+/* Save script and fun in the script/callback save vector and return
+   its index.  */
+static ptrdiff_t
+save_script_callback (struct xwidget *xw, Lisp_Object script, Lisp_Object fun)
+{
+  ptrdiff_t script_bytes = STRING_BYTES (XSTRING (script));
+  char *script_data = xmalloc (script_bytes + 1);
+  memcpy (script_data, SSDATA (script), script_bytes + 1);
+
+  ptrdiff_t idx;
+  Lisp_Object cbs = xw->script_callbacks;
+  if (NILP (cbs))
+    xw->script_callbacks = cbs = Fmake_vector (make_number (32), Qnil);
+
+  /* Find first free index.  */
+  for (idx = 0; ; idx++)
+    {
+      if (idx >= ASIZE (cbs))
+	{
+	  /* Resize script/callback save vector.  */
+	  Lisp_Object new_cbs = Fmake_vector (make_number (idx + 32), Qnil);
+	  ptrdiff_t n;
+	  for (n = 0; n < idx; n++)
+	    ASET (new_cbs, n, AREF (cbs, n));
+	  xw->script_callbacks = cbs = new_cbs;
+	}
+      if (NILP (AREF (cbs, idx)))
+	{
+	  ASET (cbs, idx, Fcons (make_save_ptr (script_data), fun));
+	  break;
+	}
+    }
+  return idx;
+}
 
 DEFUN ("xwidget-webkit-execute-script",
        Fxwidget_webkit_execute_script, Sxwidget_webkit_execute_script,
        2, 3, 0,
-       doc: /* Make the Webkit XWIDGET execute JavaScript SCRIPT.  If
-FUN is provided, feed the JavaScript return value to the single
+       doc: /* Make the Webkit XWIDGET execute JavaScript SCRIPT.
+If FUN is provided, feed the JavaScript return value to the single
 argument procedure FUN.*/)
   (Lisp_Object xwidget, Lisp_Object script, Lisp_Object fun)
 {
@@ -714,28 +753,24 @@ argument procedure FUN.*/)
   if (!NILP (fun) && !FUNCTIONP (fun))
     wrong_type_argument (Qinvalid_function, fun);
 
-  GAsyncReadyCallback callback
-    = FUNCTIONP (fun) ? webkit_javascript_finished_cb : NULL;
+  script = ENCODE_SYSTEM (script);
 
-  /* FIXME: The following hack assumes USE_LSB_TAG.  */
-  verify (USE_LSB_TAG);
-  /* FIXME: This hack might lead to disaster if FUN is garbage
-     collected before store_xwidget_js_callback_event makes it visible
-     to Lisp again.  See the FIXME in webkit_javascript_finished_cb.  */
-  gpointer callback_arg = XLP (fun);
+  /* Protect script and fun during GC.  */
+  ptrdiff_t idx = save_script_callback (xw, script, fun);
 
   /* JavaScript execution happens asynchronously.  If an elisp
      callback function is provided we pass it to the C callback
      procedure that retrieves the return value.  */
   webkit_web_view_run_javascript (WEBKIT_WEB_VIEW (xw->widget_osr),
-                                  SSDATA (script),
+                                  XSAVE_POINTER (XCAR (AREF (xw->script_callbacks, idx)), 0),
                                   NULL, /* cancelable */
-                                  callback, callback_arg);
+                                  webkit_javascript_finished_cb,
+				  (gpointer) idx);
   return Qnil;
 }
 
 DEFUN ("xwidget-resize", Fxwidget_resize, Sxwidget_resize, 3, 3, 0,
-       doc: /* Resize XWIDGET.  NEW_WIDTH, NEW_HEIGHT define the new size.  */ )
+       doc: /* Resize XWIDGET to NEW_WIDTH, NEW_HEIGHT.  */ )
   (Lisp_Object xwidget, Lisp_Object new_width, Lisp_Object new_height)
 {
   CHECK_XWIDGET (xwidget);
@@ -1197,6 +1232,16 @@ kill_buffer_xwidgets (Lisp_Object buffer)
             gtk_widget_destroy (xw->widget_osr);
             gtk_widget_destroy (xw->widgetwindow_osr);
           }
+	if (!NILP (xw->script_callbacks))
+	  {
+	    ptrdiff_t idx;
+	    for (idx = 0; idx < ASIZE (xw->script_callbacks); idx++)
+	      {
+		if (!NILP (AREF (xw->script_callbacks, idx)))
+		  xfree (XSAVE_POINTER (XCAR (AREF (xw->script_callbacks, idx)), 0));
+		ASET (xw->script_callbacks, idx, Qnil);
+	      }
+	  }
       }
     }
 }
diff --git a/src/xwidget.h b/src/xwidget.h
index 8267012d5d..93f4cfb794 100644
--- a/src/xwidget.h
+++ b/src/xwidget.h
@@ -47,6 +47,9 @@ struct xwidget
   /* A title used for button labels, for instance.  */
   Lisp_Object title;
 
+  /* Vector of currently executing scripts with callbacks.  */
+  Lisp_Object script_callbacks;
+
   /* Here ends the Lisp part.  "height" is the marker field.  */
 
   int height;
-- 
2.17.0

-- 
Andreas Schwab, [email-masked]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Subject: xwidget-webkit-execute-script does not protect script against GC
From: Andreas Schwab <[email-masked]>
To: [email-masked]
Date: Mon, 21 May 2018 14:22:53 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

The script argument to xwidget-webkit-execute-script is not protected
against GC.  Since strings may be relocated by GC the pointer passed to
webkit_web_view_run_javascript may become invalid any time during the
asynchronous execution.

Andreas.

-- 
Andreas Schwab, [email-masked]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."