[epiphany/mcatanzaro/#1445: 4/4] Fix potential loss of session state when web process is unresponsive

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 26105d63fabacb1c46525e908b8d388d223ded82
Author: Michael Catanzaro <mcatanzaro gnome org>
Date:   Thu Mar 11 19:10:25 2021 -0600

    Fix potential loss of session state when web process is unresponsive
    
    If any web process is unresponsive when the window is closed using the
    close button in the header bar -- not Ctrl+Q -- then the entire session
    state is lost. Disaster! This happens to me three or four times per
    year, and it is sad because it inevitably results in me forgetting
    important things I had to do.
    
    Anyway, the problem is our code to handle an unresponsive web process.
    We'd like to cancel the JavaScript execution using
    g_cancellable_cancel(), but it doesn't work because the underlying
    WebKit operation is not actually cancellable, so instead we use a big
    hammer and gtk_widget_destroy() the EphyWebView. Then when we wind up
    closing the session later, there are no tabs open and nothing to save.
    It was tricky to figure out how to fix this, because the only way to
    truly cancel the operation is to destroy the web view, and if we do that
    then we have to close the session first to avoid losing tabs, but the
    user could legitimately choose to reject a modified forms close
    confirmation warning, in which case they would wind up using Epiphany
    after the session is closed, meaning all subsequent changes to open tabs
    would be lost since they won't be saved after the session is closed.
    
    Eventually I realized that we can just move the timeout from EphyWindow
    -- where it's handled in two separate places, once for closing a single
    tab, and once for closing an entire window -- down into EphyWebView
    itself. This solves all our problems because now EphyWindow can assume
    the JS execution always completes in a reasonable time.
    
    I tested this by changing the call to
    webkit_web_view_run_javascript_in_world() in
    ephy_web_view_has_modified_forms() to execute a while(true) loop.
    Without the other changes, the session state is lost if closing the
    window using the close button in the header bar. With this patch, the
    session state is no longer lost.
    
    Fixes #1445

 embed/ephy-web-view.c | 38 +++++++++++++++++++++++++++
 src/ephy-window.c     | 71 +--------------------------------------------------
 2 files changed, 39 insertions(+), 70 deletions(-)
---
diff --git a/embed/ephy-web-view.c b/embed/ephy-web-view.c
index 1dd9ffc18..6daa9fee7 100644
--- a/embed/ephy-web-view.c
+++ b/embed/ephy-web-view.c
@@ -3110,6 +3110,18 @@ ephy_web_view_set_should_bypass_safe_browsing (EphyWebView *view,
   view->bypass_safe_browsing = bypass_safe_browsing;
 }
 
+static gboolean
+has_modified_forms_timeout_cb (gpointer user_data)
+{
+  GTask *task = user_data;
+
+  g_assert (!g_task_get_completed (task));
+  g_task_set_task_data (task, GINT_TO_POINTER (0), NULL);
+  g_task_return_boolean (task, FALSE);
+
+  return G_SOURCE_REMOVE;
+}
+
 static void
 has_modified_forms_cb (WebKitWebView *view,
                        GAsyncResult  *result,
@@ -3118,8 +3130,18 @@ has_modified_forms_cb (WebKitWebView *view,
   WebKitJavascriptResult *js_result;
   gboolean retval = FALSE;
   GError *error = NULL;
+  gulong id;
 
   js_result = webkit_web_view_run_javascript_in_world_finish (view, result, &error);
+
+  id = GPOINTER_TO_INT (g_task_get_task_data (task));
+  if (id == 0) {
+    /* We hit the timeout. Our task has already returned. */
+    g_assert (g_task_get_completed (task));
+    goto out;
+  }
+  g_source_remove (id);
+
   if (!js_result) {
     g_task_return_error (task, error);
   } else {
@@ -3128,6 +3150,9 @@ has_modified_forms_cb (WebKitWebView *view,
     webkit_javascript_result_unref (js_result);
   }
 
+out:
+  if (js_result)
+    webkit_javascript_result_unref (js_result);
   g_object_unref (task);
 }
 
@@ -3152,10 +3177,23 @@ ephy_web_view_has_modified_forms (EphyWebView         *view,
                                   gpointer             user_data)
 {
   GTask *task;
+  gulong id;
 
   g_assert (EPHY_IS_WEB_VIEW (view));
 
   task = g_task_new (view, cancellable, callback, user_data);
+
+  /* Set timeout to guard against web process hangs. Otherwise, a single
+   * unresponsive web process would prevent the window from closing. Note that
+   * although webkit_web_view_run_javascript_in_world() takes a cancellable,
+   * it's not *really* cancellable and attempting to cancel it just causes it to
+   * return G_IO_ERROR_CANCELLED after however long it takes to finish, which
+   * will be never if the web process is unresponsive, so we always fake
+   * completion after a two second delay.
+   */
+  id = g_timeout_add_seconds (2, has_modified_forms_timeout_cb, task);
+  g_task_set_task_data (task, GINT_TO_POINTER (id), NULL);
+
   webkit_web_view_run_javascript_in_world (WEBKIT_WEB_VIEW (view),
                                            "Ephy.hasModifiedForms();",
                                            ephy_embed_shell_get_guid (ephy_embed_shell_get_default ()),
diff --git a/src/ephy-window.c b/src/ephy-window.c
index 172c4d1ea..9fefb7d2f 100644
--- a/src/ephy-window.c
+++ b/src/ephy-window.c
@@ -2786,19 +2786,6 @@ ephy_window_close_tab (EphyWindow *window,
    * web process (or network process) has hung. E.g. the user could
    * click the close button several times. This is difficult to guard
    * against elsewhere.
-   *
-   * Even if there is only one close request,
-   * tab_has_modified_forms_timeout_cb() will run when the web process
-   * has hung, calling ephy_window_close_tab(). Then in the first call
-   * to gtk_widget_destroy() below, that will trigger the completion of
-   * a stalled call to ephy_web_view_has_modified_forms(), so
-   * tab_has_modified_forms_cb() will execute, and that also calls this
-   * function.
-   *
-   * Either way, a second call to ephy_web_view_close_tab() can be
-   * triggered recursively before gtk_widget_destroy() has completed.
-   * And it turns out that calling gtk_widget_destroy() recursively
-   * inside itself is bad. So guard against this.
    */
   if (GPOINTER_TO_INT (g_object_get_data (G_OBJECT (tab), "ephy-window-close-tab-closed")))
     return;
@@ -2834,7 +2821,6 @@ typedef struct {
   EphyWindow *window;
   EphyEmbed *embed;
   HdyTabPage *page;
-  guint id;
 } TabHasModifiedFormsData;
 
 static TabHasModifiedFormsData *
@@ -2846,7 +2832,6 @@ tab_has_modified_forms_data_new (EphyWindow *window,
   data->window = window;
   data->embed = g_object_ref (embed);
   data->page = page;
-  data->id = 0;
   g_object_add_weak_pointer (G_OBJECT (window), (gpointer *)&data->window);
   g_object_add_weak_pointer (G_OBJECT (page), (gpointer *)&data->page);
   return data;
@@ -2858,7 +2843,6 @@ tab_has_modified_forms_data_free (TabHasModifiedFormsData *data)
   g_clear_weak_pointer (&data->window);
   g_clear_object (&data->embed);
   g_clear_weak_pointer (&data->page);
-  g_clear_handle_id (&data->id, g_source_remove);
   g_clear_pointer (&data, g_free);
 }
 
@@ -2871,13 +2855,7 @@ tab_has_modified_forms_cb (EphyWebView             *view,
 
   has_modified_forms = ephy_web_view_has_modified_forms_finish (view, result, NULL);
 
-  if (data->id != 0) {
-    /* Ensure tab doesn't close while waiting for the user. */
-    g_source_remove (data->id);
-  }
-
-  if (data->id != 0 &&
-      data->window != NULL &&
+  if (data->window != NULL &&
       data->embed != NULL &&
       data->page != NULL) {
     HdyTabView *tab_view = ephy_tab_view_get_tab_view (data->window->tab_view);
@@ -2894,34 +2872,9 @@ tab_has_modified_forms_cb (EphyWebView             *view,
       hdy_tab_view_close_page_finish (tab_view, data->page, FALSE);
   }
 
-  data->id = 0;
   tab_has_modified_forms_data_free (data);
 }
 
-static gboolean
-tab_has_modified_forms_timeout_cb (TabHasModifiedFormsData *data)
-{
-  /* The web process has stalled and ephy_web_view_has_modified_forms()
-   * will probably not complete unless we destroy the tab. So do that
-   * now. Beware: this will trigger the callback to complete before it
-   * returns! So data will be freed during the call to
-   * ephy_window_close_tab().
-   */
-  data->id = 0;
-  if (data->window != NULL &&
-      data->embed != NULL &&
-      data->page != NULL) {
-    HdyTabView *tab_view = ephy_tab_view_get_tab_view (data->window->tab_view);
-    HdyTabPage *page = data->page;
-
-    ephy_window_close_tab (data->window, data->embed);
-
-    hdy_tab_view_close_page_finish (tab_view, page, TRUE);
-  }
-
-  return G_SOURCE_REMOVE;
-}
-
 static void
 run_downloads_in_background (EphyWindow *window,
                              int         num)
@@ -2989,7 +2942,6 @@ tab_view_close_page_cb (HdyTabView *tab_view,
      * been hung if there's no response after one second.
      */
     data = tab_has_modified_forms_data_new (window, embed, page);
-    data->id = g_timeout_add_seconds (1, (GSourceFunc)tab_has_modified_forms_timeout_cb, data);
     ephy_web_view_has_modified_forms (ephy_embed_get_web_view (embed),
                                       NULL,
                                       (GAsyncReadyCallback)tab_has_modified_forms_cb,
@@ -4272,21 +4224,6 @@ window_has_modified_forms_cb (EphyWebView                *view,
   window_has_modified_forms_data_free (data);
 }
 
-static gboolean
-window_has_modified_forms_timeout_cb (WindowHasModifiedFormsData *data)
-{
-  data->window->modified_forms_timeout_id = 0;
-
-  /* We have a stalled web process. Using the cancellable is not enough.
-   * Nothing for it but to force things. Each web view's
-   * has_modified_forms_cb() will complete as that view is being
-   * destroyed.
-   */
-  gtk_widget_destroy (GTK_WIDGET (data->window));
-
-  return G_SOURCE_REMOVE;
-}
-
 /* This function checks an entire EphyWindow to see if it contains any tab with
  * modified forms. There is an entirely separate codepath for checking whether
  * a single tab has modified forms, see tab_view_close_page_cb().
@@ -4314,12 +4251,6 @@ ephy_window_check_modified_forms (EphyWindow *window)
                                       data);
   }
 
-  /* Set timeout to guard against web process hangs. Otherwise, a single
-   * unresponsive web process would prevent the window from closing.
-   */
-  window->modified_forms_timeout_id =
-    g_timeout_add_seconds (1, (GSourceFunc)window_has_modified_forms_timeout_cb, data);
-
   g_list_free (tabs);
 }