Merge lp:~mhr3/libunity/fix-1063300 into lp:libunity

[Michal Hruby]

Fix possible crash when splitting uris

Fix possible crash when splitting uris.

Added test for the issue.

[Paweł Stołowski (stolowski)]

+ Test.log_set_fatal_handler (() => { return false; });

Does this affect other test cases?

review: Needs Information

[Michal Hruby (mhr3)]

> + Test.log_set_fatal_handler (() => { return false; });
>
> Does this affect other test cases?

From devhelp:
Note that the handler is reset at the beginning of any test case, so you have to set it inside each test function which needs the special behavior.

[Paweł Stołowski (stolowski)]

That's great. Just one suggestion - could you please use Unity.Protocol.ActionType enum for call_lens_activate argument, instead of 0/1?

review: Needs Fixing

[lp:~mhr3/libunity/fix-1063300]

191. By Michal Hruby on 2012-10-15

Act on review comments

[Michal Hruby (mhr3)]

> That's great. Just one suggestion - could you please use
> Unity.Protocol.ActionType enum for call_lens_activate argument, instead of
> 0/1?

Done.

=== modified file 'protocol/protocol-lens-interface.vala'
--- protocol/protocol-lens-interface.vala	2012-09-04 11:40:42 +0000
+++ protocol/protocol-lens-interface.vala	2012-10-15 16:53:20 +0000
@@ -38,6 +38,15 @@
   public HashTable<string, Variant> hints;
 }
 
+/* The error types that can be thrown from DBus methods */
+[DBus (name = "com.canonical.Unity.LensError")]
+public errordomain LensError
+{
+  FAILED,
+  INVALID_SCOPE
+}
+
+
 public enum HandledType
 {
   NOT_HANDLED,
@@ -74,12 +83,12 @@
 
   // FIXME: should have the hints param and activate_with_hints should go away
   public async abstract ActivationReplyRaw activate (
-      string uri, uint action_type) throws IOError;
+      string uri, uint action_type) throws IOError, LensError;
 
   public async abstract ActivationReplyRaw activate_with_hints (
-      string uri, uint action_type, HashTable<string, Variant> hints) throws IOError;
+      string uri, uint action_type, HashTable<string, Variant> hints) throws IOError, LensError;
 
-  public async abstract HashTable<string, Variant> update_preview_property (string uri, HashTable<string, Variant> values) throws IOError;
+  public async abstract HashTable<string, Variant> update_preview_property (string uri, HashTable<string, Variant> values) throws IOError, LensError;
 
   public async abstract HashTable<string, Variant> search (
       string search_string, HashTable<string, Variant> hints) throws IOError;

=== modified file 'src/unity-lens-private.vala'
--- src/unity-lens-private.vala	2012-09-04 11:40:42 +0000
+++ src/unity-lens-private.vala	2012-10-15 16:53:20 +0000
@@ -493,8 +493,8 @@
     queue_info_changed ();
   }
 
-  public async ActivationReplyRaw activate (string uri,
-                                            uint action_type) throws IOError
+  public async ActivationReplyRaw activate (
+      string uri, uint action_type) throws IOError, LensError
   {
     var hints = new HashTable<string, Variant> (null, null);
     var reply = yield activate_with_hints (uri, action_type, hints);
@@ -503,7 +503,7 @@
 
   public async ActivationReplyRaw activate_with_hints (
       string uri, uint action_type,
-      HashTable<string, Variant> hints) throws IOError
+      HashTable<string, Variant> hints) throws IOError, LensError
   {
     string[] tokens;
     ScopeProxy? scope = null;
@@ -513,11 +513,15 @@
     {
       case ActionType.PREVIEW_ACTION:
         tokens = uri.split (":", 3);
+        if (tokens.length < 3) 
+          throw new LensError.FAILED ("Malformatted URI parameter");
         scope = get_scope_for_uid (tokens[1]);
         scope_uri = "%s:%s".printf (tokens[0], tokens[2]);
         break;
       default:
         tokens = uri.split (":", 2);
+        if (tokens.length < 2)
+          throw new LensError.FAILED ("Malformatted URI parameter");
         scope = get_scope_for_uid (tokens[0]);
         scope_uri = tokens[1];
         break;
@@ -535,7 +539,7 @@
     else
     {
       warning ("Unable to find scope for activation request: %s", uri);
-      raw.uri = uri;
+      throw new LensError.INVALID_SCOPE ("Invalid URI parameter");
     }
 
     // FIXME: do we need to mangle uris/actions that the scope provided?
@@ -546,11 +550,14 @@
     return raw;
   }
 
-  public async HashTable<string, Variant> update_preview_property (string uri,
-                                             HashTable<string, Variant> values) throws IOError
+  public async HashTable<string, Variant> update_preview_property (
+      string uri, HashTable<string, Variant> values) throws IOError, LensError
   {
     string[] tokens = uri.split (":", 2);
-    string? scope_uri = tokens[1];
+    if (tokens.length < 2)
+      throw new LensError.FAILED ("Malformatted URI parameter");
+
+    string scope_uri = tokens[1];
     ScopeProxy? scope = get_scope_for_uid (tokens[0]);
     
     if (scope is ScopeProxy)
@@ -561,6 +568,7 @@
     else
     {
       warning ("Unable to find scope for activation request: %s", uri);
+      throw new LensError.INVALID_SCOPE ("Invalid URI parameter");
     }
 
     return new HashTable<string, Variant> (str_hash, str_equal);

=== modified file 'test/vala/test-lens.vala'
--- test/vala/test-lens.vala	2012-10-02 16:16:29 +0000
+++ test/vala/test-lens.vala	2012-10-15 16:53:20 +0000
@@ -54,6 +54,7 @@
     Test.add_data_func ("/Unit/Lens/ReplyHint", test_lens_reply_hint);
     Test.add_data_func ("/Unit/Lens/Sources", test_lens_sources);
     Test.add_data_func ("/Unit/Lens/Activation", test_lens_activation);
+    Test.add_data_func ("/Unit/Lens/InvalidActivation", test_lens_invalid_activation);
     Test.add_data_func ("/Unit/Lens/Preview", test_lens_preview);
     Test.add_data_func ("/Unit/Lens/Preview/Async", test_lens_preview_async);
     Test.add_data_func ("/Unit/Lens/Preview/AsyncWithNull", test_lens_preview_async_with_null);
@@ -163,9 +164,10 @@
     assert (scope_up == true);
   }
 
-  private static void call_lens_method (string method_name, 
+  private static void call_lens_method (string method_name,
                                         Variant? parameters,
-                                        Func<Variant?>? cb)
+                                        Func<Variant?>? cb,
+                                        Func<Error?>? error_cb = null)
   {
     DBusConnection? bus = null;
     try
@@ -190,35 +192,43 @@
         var reply = bus.call.end (res);
         if (cb != null) cb (reply);
       }
-      catch (Error e)
+      catch (Error err)
       {
-        warning ("%s", e.message);
+        if (error_cb != null)
+          error_cb (err);
+        else
+          warning ("%s", err.message);
       }
     });
   }
 
   private static void call_lens_search (string search_string,
-                                        Func<Variant?>? cb = null)
+                                        Func<Variant?>? cb = null,
+                                        Func<Error?>? error_cb = null)
   {
     var vb = new VariantBuilder (new VariantType ("(sa{sv})"));
     vb.add ("s", search_string);
     vb.open (new VariantType ("a{sv}"));
     vb.close ();
 
-    call_lens_method ("Search", vb.end (), cb);
+    call_lens_method ("Search", vb.end (), cb, error_cb);
   }
 
-  private static void call_lens_activate (string uri, uint action_type,
-                                          Func<Variant?>? cb = null)
+  private static void call_lens_activate (string uri,
+                                          Unity.Protocol.ActionType action_type,
+                                          Func<Variant?>? cb = null,
+                                          Func<Error?>? error_cb = null)
   {
     call_lens_activate_with_hints (uri, action_type,
                                    new HashTable<string, Variant> (null, null),
-                                   cb);
+                                   cb, error_cb);
   }
 
   private static void call_lens_activate_with_hints (
-      string uri, uint action_type, HashTable<string, Variant> hints,
-      Func<Variant?>? cb = null)
+      string uri, Unity.Protocol.ActionType action_type,
+      HashTable<string, Variant> hints,
+      Func<Variant?>? cb = null,
+      Func<Error?>? error_cb = null)
   {
     Variant parameters;
     // let's make sure we test both variants, since we have to support them
@@ -227,24 +237,25 @@
       Variant ht = hints;
       parameters = new Variant ("(su@a{sv})", uri, action_type, ht);
 
-      call_lens_method ("ActivateWithHints", parameters, cb);
+      call_lens_method ("ActivateWithHints", parameters, cb, error_cb);
     }
     else
     {
       parameters = new Variant ("(su)", uri, action_type);
 
-      call_lens_method ("Activate", parameters, cb);
+      call_lens_method ("Activate", parameters, cb, error_cb);
     }
   }
 
   private static void call_lens_update_preview_property (string uri, HashTable<string, Variant> props,
-                                                         Func<Variant?>? cb = null)
+                                                         Func<Variant?>? cb = null,
+                                                         Func<Error?>? error_cb = null)
   {
     var vb = new VariantBuilder (new VariantType ("(sa{sv})"));
     vb.add ("s", uri);
     vb.add_value (props);
 
-    call_lens_method ("UpdatePreviewProperty", vb.end(), cb);
+    call_lens_method ("UpdatePreviewProperty", vb.end(), cb, error_cb);
   }
     
   public static void test_lens_search ()
@@ -747,7 +758,8 @@
     }));
 
     ml = new MainLoop ();
-    call_lens_activate (mangled_uri, 0, () =>
+    var action = Unity.Protocol.ActionType.ACTIVATE_RESULT;
+    call_lens_activate (mangled_uri, action, () =>
     {
       ml.quit ();
     });
@@ -756,6 +768,28 @@
     assert (got_activate_uri_signal);
   }
 
+  public static void test_lens_invalid_activation ()
+  {
+    // ignore warnings
+    Test.log_set_fatal_handler (() => { return false; });
+    Unity.Protocol.ActionType action;
+    var ml = new MainLoop ();
+
+    Func<Error?> err_cb = (err) =>
+    {
+      assert (err is Unity.Protocol.LensError);
+      ml.quit ();
+    };
+    action = Unity.Protocol.ActionType.ACTIVATE_RESULT;
+    call_lens_activate ("", action, null, err_cb);
+    assert (run_with_timeout (ml, 5000));
+
+    ml = new MainLoop ();
+    action = Unity.Protocol.ActionType.PREVIEW_RESULT;
+    call_lens_activate (":", action, null, err_cb);
+    assert (run_with_timeout (ml, 5000));
+  }
+
   public static void test_lens_preview ()
   {
     SignalWrapper[] signals = null;
@@ -792,7 +826,8 @@
 
     ml = new MainLoop ();
     Unity.Protocol.Preview? reconstructed = null;
-    call_lens_activate (mangled_uri, 1, (reply) =>
+    var action_type = Unity.Protocol.ActionType.PREVIEW_RESULT;
+    call_lens_activate (mangled_uri, action_type, (reply) =>
     {
       var v = reply.get_child_value (0);
       Unity.Protocol.ActivationReplyRaw reply_struct =
@@ -816,7 +851,9 @@
 
     // expecting button_id:scope_uid:uri
     ml = new MainLoop ();
-    call_lens_activate ("%s:%s".printf (action.id, mangled_uri), 2, (reply) =>
+    action_type = Unity.Protocol.ActionType.PREVIEW_ACTION;
+    var activate_uri = "%s:%s".printf (action.id, mangled_uri);
+    call_lens_activate (activate_uri, action_type, (reply) =>
     {
       ml.quit ();
     });
@@ -871,7 +908,8 @@
 
     ml = new MainLoop ();
     Unity.Protocol.Preview? reconstructed = null;
-    call_lens_activate (mangled_uri, 1, (reply) =>
+    var action_type = Unity.Protocol.ActionType.PREVIEW_RESULT;
+    call_lens_activate (mangled_uri, action_type, (reply) =>
     {
       var v = reply.get_child_value (0);
       Unity.Protocol.ActivationReplyRaw reply_struct =
@@ -897,7 +935,9 @@
 
     // expecting button_id:scope_uid:uri
     ml = new MainLoop ();
-    call_lens_activate ("%s:%s".printf (action.id, mangled_uri), 2, (reply) =>
+    action_type = Unity.Protocol.ActionType.PREVIEW_ACTION;
+    var activate_uri = "%s:%s".printf (action.id, mangled_uri);
+    call_lens_activate (activate_uri, action_type, (reply) =>
     {
       ml.quit ();
     });
@@ -945,7 +985,8 @@
 
     ml = new MainLoop ();
     Unity.Protocol.Preview? reconstructed = null;
-    call_lens_activate (mangled_uri, 1, (reply) =>
+    var action = Unity.Protocol.ActionType.PREVIEW_RESULT;
+    call_lens_activate (mangled_uri, action, (reply) =>
     {
       var v = reply.get_child_value (0);
       Unity.Protocol.ActivationReplyRaw reply_struct =
@@ -979,7 +1020,6 @@
     bool got_preview_uri_signal = false;
     string? request_item_signal_uri = null;
     int request_item_signal_count = 0;
-    Unity.PreviewAction? action = null;
 
     signals += new SignalWrapper (local_scope,
       local_scope.activate_uri.connect ((uri) =>
@@ -1113,7 +1153,8 @@
     var mangled_uri = lens_results_model.get_string (iter, 0);
 
     Unity.Protocol.GenericPreview? reconstructed = null;
-    call_lens_activate (mangled_uri, Unity.Protocol.ActionType.PREVIEW_RESULT, (reply) =>
+    var action = Unity.Protocol.ActionType.PREVIEW_RESULT;
+    call_lens_activate (mangled_uri, action, (reply) =>
     {
       var v = reply.get_child_value (0);
       Unity.Protocol.ActivationReplyRaw reply_struct =
@@ -1167,7 +1208,7 @@
     {
       got_activated_signal = true;
       assert (action.hints["passing-extra-info"].get_boolean () == true);
-      return null;
+      return new Unity.ActivationResponse (Unity.HandledType.NOT_HANDLED);
     }));
 
     // this signal handler gets called when activate is called with PREVIEW_RESULT
@@ -1187,7 +1228,8 @@
     var mangled_uri = lens_results_model.get_string (iter, 0);
 
     Unity.Protocol.Preview? reconstructed = null;
-    call_lens_activate (mangled_uri, Unity.Protocol.ActionType.PREVIEW_RESULT, (reply) =>
+    var action_type = Unity.Protocol.ActionType.PREVIEW_RESULT;
+    call_lens_activate (mangled_uri, action_type, (reply) =>
     {
       var v = reply.get_child_value (0);
       Unity.Protocol.ActivationReplyRaw reply_struct =