[glib/wip/tingping/network-address-errors: 1/2] gnetworkaddress: Fix incorrect error propagation when resolving addresses

[Patrick Griffis <pgriffis src gnome org>]

commit 690624c9e6f44e0f40073d6c0acd26f07e1e4e85
Author: Patrick Griffis <tingping tingping se>
Date:   Thu Feb 7 09:50:56 2019 -0500

    gnetworkaddress: Fix incorrect error propagation when resolving addresses
    
    Previously this would always error if ipv6 errored after ipv4
    succeeded which was incorrect.
    
    This explicitly tests the order of erroring.
    
    Fixes #1644

 gio/gnetworkaddress.c       | 27 +++++++++++------
 gio/tests/network-address.c | 70 ++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 79 insertions(+), 18 deletions(-)
---
diff --git a/gio/gnetworkaddress.c b/gio/gnetworkaddress.c
index 60736874e..e81d311ce 100644
--- a/gio/gnetworkaddress.c
+++ b/gio/gnetworkaddress.c
@@ -38,6 +38,10 @@
 
 #include <string.h>
 
+/* As recommended by RFC 8305 this is the time it waits for a following
+   DNS response to come in (ipv4 waiting on ipv6 generally)
+ */
+#define HAPPY_EYEBALLS_RESOLUTION_DELAY_MS 50
 
 /**
  * SECTION:gnetworkaddress
@@ -1143,6 +1147,7 @@ got_ipv6_addresses (GObject      *source_object,
   GResolver *resolver = G_RESOLVER (source_object);
   GList *addresses;
   GError *error = NULL;
+  gboolean got_ipv4_first = FALSE;
 
   addresses = g_resolver_lookup_by_name_with_flags_finish (resolver, result, &error);
   if (!error)
@@ -1161,18 +1166,18 @@ got_ipv6_addresses (GObject      *source_object,
     {
       g_source_destroy (addr_enum->wait_source);
       g_clear_pointer (&addr_enum->wait_source, g_source_unref);
+      got_ipv4_first = TRUE;
     }
 
-  /* If we got an error before ipv4 then let it handle it.
+  /* If we got an error before ipv4 then let its response handle it.
    * If we get ipv6 response first or error second then
    * immediately complete the task.
    */
-  if (error != NULL && !addr_enum->last_error)
+  if (error != NULL && !addr_enum->last_error && !got_ipv4_first)
     {
       addr_enum->last_error = g_steal_pointer (&error);
 
-      /* This shouldn't happen often but avoid never responding. */
-      addr_enum->wait_source = g_timeout_source_new_seconds (1);
+      addr_enum->wait_source = g_timeout_source_new (HAPPY_EYEBALLS_RESOLUTION_DELAY_MS);
       g_source_set_callback (addr_enum->wait_source,
                              on_address_timeout,
                              addr_enum, NULL);
@@ -1180,13 +1185,19 @@ got_ipv6_addresses (GObject      *source_object,
     }
   else if (addr_enum->queued_task != NULL)
     {
+      GError *task_error = NULL;
+
+      /* If both errored just use the ipv6 one,
+         but if ipv6 errored and ipv4 didn't we don't error */
+      if (error != NULL && addr_enum->last_error)
+          task_error = g_steal_pointer (&error);
+
       g_clear_error (&addr_enum->last_error);
       complete_queued_task (addr_enum, g_steal_pointer (&addr_enum->queued_task),
-                            g_steal_pointer (&error));
+                            g_steal_pointer (&task_error));
     }
-  else if (error != NULL)
-    g_clear_error (&error);
 
+  g_clear_error (&error);
   g_object_unref (addr_enum);
 }
 
@@ -1229,7 +1240,7 @@ got_ipv4_addresses (GObject      *source_object,
   else if (addr_enum->queued_task != NULL)
     {
       addr_enum->last_error = g_steal_pointer (&error);
-      addr_enum->wait_source = g_timeout_source_new (50);
+      addr_enum->wait_source = g_timeout_source_new (HAPPY_EYEBALLS_RESOLUTION_DELAY_MS);
       g_source_set_callback (addr_enum->wait_source,
                              on_address_timeout,
                              addr_enum, NULL);
diff --git a/gio/tests/network-address.c b/gio/tests/network-address.c
index 4a2718e24..4605f8eeb 100644
--- a/gio/tests/network-address.c
+++ b/gio/tests/network-address.c
@@ -710,17 +710,18 @@ test_happy_eyeballs_slow_connection_and_ipv4 (HappyEyeballsFixture *fixture,
 }
 
 static void
-test_happy_eyeballs_ipv6_error (HappyEyeballsFixture *fixture,
-                                gconstpointer         user_data)
+test_happy_eyeballs_ipv6_error_ipv4_first (HappyEyeballsFixture *fixture,
+                                           gconstpointer         user_data)
 {
   AsyncData data = { 0 };
   GError *ipv6_error;
 
-  /* If ipv6 fails we still get ipv4. */
+  /* If ipv6 fails, ensuring that ipv4 finishes before ipv6 errors, we still get ipv4. */
 
   data.loop = fixture->loop;
   ipv6_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv6 Broken");
   mock_resolver_set_ipv6_error (fixture->mock_resolver, ipv6_error);
+  mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, 25);
 
   g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data);
   g_main_loop_run (fixture->loop);
@@ -731,17 +732,62 @@ test_happy_eyeballs_ipv6_error (HappyEyeballsFixture *fixture,
 }
 
 static void
-test_happy_eyeballs_ipv4_error (HappyEyeballsFixture *fixture,
-                                gconstpointer         user_data)
+test_happy_eyeballs_ipv6_error_ipv6_first (HappyEyeballsFixture *fixture,
+                                           gconstpointer         user_data)
+{
+  AsyncData data = { 0 };
+  GError *ipv6_error;
+
+  /* If ipv6 fails, ensuring that ipv6 errors before ipv4 finishes, we get an error. */
+
+  data.loop = fixture->loop;
+  ipv6_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv6 Broken");
+  mock_resolver_set_ipv6_error (fixture->mock_resolver, ipv6_error);
+  mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, 25);
+
+  g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data);
+  g_main_loop_run (fixture->loop);
+
+  assert_list_matches_expected (data.addrs, fixture->input_ipv4_results);
+
+  g_error_free (ipv6_error);
+}
+
+static void
+test_happy_eyeballs_ipv4_error_ipv4_first (HappyEyeballsFixture *fixture,
+                                           gconstpointer         user_data)
 {
   AsyncData data = { 0 };
   GError *ipv4_error;
 
-  /* If ipv4 fails we still get ipv6. */
+  /* If ipv4 fails, ensuring that ipv4 errors before ipv6 finishes, we still get ipv6. */
 
   data.loop = fixture->loop;
   ipv4_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv4 Broken");
   mock_resolver_set_ipv4_error (fixture->mock_resolver, ipv4_error);
+  mock_resolver_set_ipv6_delay_ms (fixture->mock_resolver, 25);
+
+  g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data);
+  g_main_loop_run (fixture->loop);
+
+  assert_list_matches_expected (data.addrs, fixture->input_ipv6_results);
+
+  g_error_free (ipv4_error);
+}
+
+static void
+test_happy_eyeballs_ipv4_error_ipv6_first (HappyEyeballsFixture *fixture,
+                                           gconstpointer         user_data)
+{
+  AsyncData data = { 0 };
+  GError *ipv4_error;
+
+  /* If ipv4 fails, ensuring that ipv6 finishes before ipv4 errors, we still get ipv6. */
+
+  data.loop = fixture->loop;
+  ipv4_error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_TIMED_OUT, "IPv4 Broken");
+  mock_resolver_set_ipv4_error (fixture->mock_resolver, ipv4_error);
+  mock_resolver_set_ipv4_delay_ms (fixture->mock_resolver, 25);
 
   g_socket_address_enumerator_next_async (fixture->enumerator, NULL, got_addr, &data);
   g_main_loop_run (fixture->loop);
@@ -913,10 +959,14 @@ main (int argc, char *argv[])
               happy_eyeballs_setup, test_happy_eyeballs_very_slow_ipv6, happy_eyeballs_teardown);
   g_test_add ("/network-address/happy-eyeballs/slow-connection-and-ipv4", HappyEyeballsFixture, NULL,
               happy_eyeballs_setup, test_happy_eyeballs_slow_connection_and_ipv4, happy_eyeballs_teardown);
-  g_test_add ("/network-address/happy-eyeballs/ipv6-error", HappyEyeballsFixture, NULL,
-              happy_eyeballs_setup, test_happy_eyeballs_ipv6_error, happy_eyeballs_teardown);
-  g_test_add ("/network-address/happy-eyeballs/ipv4-error", HappyEyeballsFixture, NULL,
-              happy_eyeballs_setup, test_happy_eyeballs_ipv4_error, happy_eyeballs_teardown);
+  g_test_add ("/network-address/happy-eyeballs/ipv6-error-ipv4-first", HappyEyeballsFixture, NULL,
+              happy_eyeballs_setup, test_happy_eyeballs_ipv6_error_ipv4_first, happy_eyeballs_teardown);
+  g_test_add ("/network-address/happy-eyeballs/ipv6-error-ipv6-first", HappyEyeballsFixture, NULL,
+              happy_eyeballs_setup, test_happy_eyeballs_ipv6_error_ipv6_first, happy_eyeballs_teardown);
+  g_test_add ("/network-address/happy-eyeballs/ipv4-error-ipv6-first", HappyEyeballsFixture, NULL,
+              happy_eyeballs_setup, test_happy_eyeballs_ipv4_error_ipv6_first, happy_eyeballs_teardown);
+  g_test_add ("/network-address/happy-eyeballs/ipv4-error-ipv4-first", HappyEyeballsFixture, NULL,
+              happy_eyeballs_setup, test_happy_eyeballs_ipv4_error_ipv4_first, happy_eyeballs_teardown);
   g_test_add ("/network-address/happy-eyeballs/both-error", HappyEyeballsFixture, NULL,
               happy_eyeballs_setup, test_happy_eyeballs_both_error, happy_eyeballs_teardown);
   g_test_add ("/network-address/happy-eyeballs/both-error-delays-1", HappyEyeballsFixture, NULL,