Merge lp://qastaging/~3v1n0/unity/lim into lp://qastaging/unity

I've just fixed few small issues on the code, and tested it on the real world. The core part works as expected, so I think it can be landed if we need it in trunk for unity-2d.
More tests will arrive soon by the way.

862 + if (sscanf (entry_id, "%p", &entry) == 1)
863 + {
864 + /* Checking that entry is really an IndicatorObjectEntry.
865 + * To do that, we use an hack that allows to check if the pointer we read is
866 + * actually a valid pointer without crashing. This can be done using the
867 + * low-level write function to read from the pointer to a valid fds (we use
868 + * a pipe for convenience). Write will fail without crashing if we try to
869 + * read from an invalid pointer, so we can finally be pretty sure if the
870 + * pointed entry is an IndicatorObjectEntry by checking if it has a valid
871 + * parent IndicatorObject */
872 +
873 + int fds[2];
874 +
875 + if (pipe (fds) > -1)
876 + {
877 + size_t data_size = sizeof (IndicatorObjectEntry);
878 +
879 + if (write (fds[1], entry, data_size) != data_size)
880 + {
881 + entry = NULL;
882 + }
883 + else
884 + {
885 + data_size = sizeof (IndicatorObject*);
886 +
887 + if (write (fds[1], entry->parent_object, data_size) != data_size ||
888 + !INDICATOR_IS_OBJECT (entry->parent_object))
889 + {
890 + entry = NULL;
891 + }
892 + }
893 +
894 + close (fds[0]);
895 + close (fds[1]);
896 + }
897 + }

Clever, but, why are we even checking for the validity of a pointer here ? Are we passing pointers around in dbus ?

> 862 + if (sscanf (entry_id, "%p", &entry) == 1)
> 863 + {
> 864 + /* Checking that entry is really an IndicatorObjectEntry.
> [...]
>
> Clever, but, why are we even checking for the validity of a pointer here ? Are
> we passing pointers around in dbus ?

Yes.
That's what we've done so far. Actually they're just unique ID, but since they're pointers in fact we can just use these ids to quickly get the struct we're referring to, without using other data to store them that would increase the computation and the memory used (and considering the overhead that dbus already causes to us when scrubbing menus, the faster we are, the best it is). Since we can be more smart (and safe), using low-level things we use them.

Also, since the IndicatorObjectEntry's are only structs and not GObject's, this is the only way we have to check if we're really working with a valid one.

On 1 February 2012 17:40, Marco Trevisan (Treviño) <mail@3v1n0.net> wrote:
>> 862     + if (sscanf (entry_id, "%p", &entry) == 1)
>> 863     + {
>> 864     + /* Checking that entry is really an IndicatorObjectEntry.
>> [...]
>>
>> Clever, but, why are we even checking for the validity of a pointer here ? Are
>> we passing pointers around in dbus ?
>
> Yes.
> That's what we've done so far. Actually they're just unique ID, but since they're pointers in fact we can just use these ids to quickly get the struct we're referring to, without using other data to store them that would increase the computation and the memory used (and considering the overhead that dbus already causes to us when scrubbing menus, the faster we are, the best it is). Since we can be more smart (and safe), using low-level things we use them.
>
> Also, since the IndicatorObjectEntry's are only structs and not GObject's, this is the only way we have to check if we're really working with a valid one.

There's no way to know that, and the pipe() trick does not ensure that
either. If the IndicatorObjectEntry is allocated with the slice
allocated (fx.) it'll still be valid memory even after freed, but
might contain garbage.

I am not entirely understanding the reasoning here. Is it that
creating a pipe and checking the write is cheaper than a hashtable
lookup? I doubt it as pipe(), write(), and close() requires a few mode
switches. And arguably I'd consider the hashtable trick more safe.

(although the pipe trick is neat, I am also wondering if there
wouldn't be some lower level call that checked this directly..? not
arguing in the favour of that though :-))

First of all, using pipe() is just done for convenience (to quickly get a writable fd), you can do that also using open with any valid temporary file (but not /dev/null, since it's a special file).

> > Also, since the IndicatorObjectEntry's are only structs and not GObject's,
> this is the only way we have to check if we're really working with a valid
> one.
>
> There's no way to know that, and the pipe() trick does not ensure that
> either. If the IndicatorObjectEntry is allocated with the slice
> allocated (fx.) it'll still be valid memory even after freed, but
> might contain garbage.

Yes, but... The fact is: if the pointer we found is pointing to an area of memory where we have no rights, without using this trick we'd crash. Using this trick instead, if that pointer is not on a valid area, write won't be able to read from it and instead of crashing it will give an EFAULT error.

Now, if that area is readable, write won't give any error, but at that point INDICATOR_IS_OBJECT will be able to check the pointed object without crashing, since it's in a valid area...
I've made tests locally to see if this was working on a manually allocated area (with garbage too) or even checking "self", and all the results are with me, because if INDICATOR_IS_OBJECT (and so g_type_check_instance()) will fail, is very hard that entry is valid.

> I am not entirely understanding the reasoning here. Is it that
> creating a pipe and checking the write is cheaper than a hashtable
> lookup? I doubt it as pipe(), write(), and close() requires a few mode
> switches. And arguably I'd consider the hashtable trick more safe.

Well, on this side I think that is hard to be cheaper than this... Especially on memory side.
Also, that method is not crash-free by design... Here we can be sure that no crash will happen.

> (although the pipe trick is neat, I am also wondering if there
> wouldn't be some lower level call that checked this directly..? not
> arguing in the favour of that though :-))

No... I guess we should improve glib! :)

how often is get_indicator_entry_by_id() called? I am really very cautious of creating pipes all over the place...

Also, you should be logging a critical if we're passed an invalid pointer. This means that there is a bug in Unity or someone playing tricks on us.

> how often is get_indicator_entry_by_id() called? I am really very cautious of
> creating pipes all over the place...

Every time we get a signal from the unity panel to open a menu, to scroll or to middle-click... So basically on clicks over menus, scrubbing, scrolling or middle-clicking. Plus when the gometries of an entry changes (on added/removed, and when its position on the screen change).
So this mostly happen when there's user interaction.

As said, if you think it's better, we can also just create a temporary file when the service starts, open it and keeping its fd until we stop the panel service... Using that for our pointer-checking work.

Anyway I guess that generally our system really opens a lot of fd's when doing more high-level operations (for DBus for example, but also for more standard things), that we don't care about...

> Also, you should be logging a critical if we're passed an invalid pointer.
> This means that there is a bug in Unity or someone playing tricks on us.

Yes, I agree about that. It was already on my TODO list ;)

Hi,

+static IndicatorObjectEntry *
1025 +get_indicator_entry_by_id (PanelService *self, const gchar *entry_id)
1026 +{
1027 + IndicatorObjectEntry *entry;
1028 + entry = NULL;
1029 +
1030 + if (sscanf (entry_id, "%p", &entry) == 1)
1031 + {
1032 + /* Checking that entry is really an IndicatorObjectEntry.
1033 + * To do that, we use an hack that allows to check if the pointer we read is
1034 + * actually a valid pointer without crashing. This can be done using the
1035 + * low-level write function to read from the pointer to a valid fds (we use
1036 + * a pipe for convenience). Write will fail without crashing if we try to
1037 + * read from an invalid pointer, so we can finally be pretty sure if the
1038 + * pointed entry is an IndicatorObjectEntry by checking if it has a valid
1039 + * parent IndicatorObject */
1040 +
1041 + int fds[2];
1042 +
1043 + if (pipe (fds) > -1)
1044 + {
1045 + size_t data_size = sizeof (IndicatorObjectEntry);
1046 +
1047 + if (write (fds[1], entry, data_size) != data_size)
1048 + {
1049 + entry = NULL;
1050 + }
1051 + else
1052 + {
1053 + if (g_slist_find (self->priv->indicators, entry->parent_object))
1054 + {
1055 + if (!INDICATOR_IS_OBJECT (entry->parent_object))
1056 + entry = NULL;
1057 + }
1058 + else
1059 + {
1060 + entry = NULL;
1061 + }
1062 + }
1063 +
1064 + close (fds[0]);
1065 + close (fds[1]);
1066 + }
1067 + else
1068 + {
1069 + entry = NULL;
1070 + }
1071 + }
1072 +
1073 + if (!entry)
1074 + {
1075 + g_warning("The entry id '%s' you're trying to parse is not a valid IndicatorObjectEntry!", entry_id);
1076 + }
1077 +
1078 + return entry;
1079 }
1080

I'm sorry, please don't land this code. There are several things wrong with it:

1) You're using pointers as a unique ID. That's fine - since pointers to unique objects are a good way to generate a unique id. Doing ptr -> int is fine, doing int -> ptr is not. Besides, you're now assuming that the source of your unique IDs is never going to change; if for some reason it does change, you now need to update this code as well.

2) The code needs a big comment to explain what it does. That's usually a prettyy good clue that the code needs to be made more obvious.

3) As Mikkel already pointed out, you're *assuming* that this hack has better performance, which may not be the case, since IO will involve several kernel-mode switches. At the very least, if you *really* need the performance, you should have done some profiling of the entire codebase, figured out which bits needed improving, and fixed those. Even then, I'd like to see a comparative speed test between this code and a hash table lookup (which should, of course, be constant-time and pretty darn fast for any sensible hash-table implementation).

4) As Mikkel also already pointed out, this method may yield false positives, so it doesn't even work as advertised.

I'd suggest that the correct method here is to read the id, check if it's in your hash table. If it is, do a table lookup to retrieve the value. Simple!

Cheers

Ok, I've removed the "evil" hack.
Even if I think that it was safe, I must say that it was slower.

I've made some tests, and you're right.
I've tried to put it in because I liked the idea :P

The panel is now safe again.

> I've made some tests, and you're right.
> I've tried to put it in because I liked the idea :P

That's always tempting ;)

I've spotted a few small things:

1) Please rename either the 'panel_service_show_entry' or the 'panel_service_actually_show_entry' functions. If 'panel_service_show_entry' does not show the entry then the function needs to be renamed.

2) Another naming thing: PanelIndicatorEntryView::IsVisible is misleading,s ince it doesn't check whether the PanelIndivatorEntryView is visible or not, it check's whether the remote object is visible or not. Some alternative names might be:

IsProxyVisible
IsRemoteVisible

etc.

Otherwise, the code looks good, although the panel service code makes we want to stab myself in the eye.

> 1) Please rename either the 'panel_service_show_entry' or the
> 'panel_service_actually_show_entry' functions. If 'panel_service_show_entry'
> does not show the entry then the function needs to be renamed.

The fact is that panel_service_show_entry is the public function used when the related method is called, while the panel_service_actually_show_entry is only a static function that I renamed to make common things.
I'll see if I can find a better name, but it makes sense to me anyway.

> 2) Another naming thing: PanelIndicatorEntryView::IsVisible is misleading,s
> ince it doesn't check whether the PanelIndivatorEntryView is visible or not,
> it check's whether the remote object is visible or not. Some alternative names
> might be:
>
> IsProxyVisible
> IsRemoteVisible

No, I don't want to do that, because the View is actually a kind of wrapper of the remote entry, when the remote entry is not visible, neither the view will be and back. Also all the current methods of that class refers to the view, even if they're currently using the values of the remote value. The fact that we have a remote value is a private thing, and we shouldn't care about that.

For the record, I've put this on WIP again because we need to include a new parameter to the EntryActivate signal to include the geometry of that entry.

44 +const std::string SETTING_NAME("com.canonical.indicator.appmenu");
45 +const std::string SET
TING_KEY("menu-mode");

Maybe they shuld be in a unnamed namespace?

---

48 + : Indicator(name),
49 + gsettings_(g_settings_new(SETTING_NAME.c_str())),
50 + integrated_(false)

For the next time... :

48 + : Indicator(name)
49 + , gsettings_(g_settings_new(SETTING_NAME.c_str()))
50 + , integrated_(false)

---

setting_changed_.Connect(gsettings_, "changed::menu-mode", [&] (GSettings*, gchar* key) {
53 + CheckSettingValue();
54 + });

If you're not using the argument gchar* key, please use an unnamed variable too...

---

g_idle_add_full (G_PRIORITY_DEFAULT, (GSourceFunc) send_show_entry, data, NULL);
g_variant_get (parameters, "(s(iiuu))", &entry_name, &geo.x, &geo.y, &geo.width, &geo.height);

Remove the whitespace between the function name and the bracket.

---

AppmenuIndicator *appmenu = dynamic_cast<AppmenuIndicator*>(indicator.get());

:(

---

751 - * Copyright (C) 2010 Canonical Ltd
752 + * Copyright (C) 2012 Canonical Ltd

Please do 2010-2012

---

760 +#include <NuxCore/Rect.h>

I think you can use a forward declaration.

---

+ bool IsVisible();

Const?

---+ bool IsVisible();

Btw all of these are not blocking ;)

> 44 +const std::string SETTING_NAME("com.canonical.indicator.appmenu");
> 45 +const std::string SET
> TING_KEY("menu-mode");
>
> Maybe they shuld be in a unnamed namespace?

Right. I thought I had used.

> 48 + : Indicator(name),
> 49 + gsettings_(g_settings_new(SETTING_NAME.c_str())),
> 50 + integrated_(false)
>
> For the next time... :
>
> 48 + : Indicator(name)
> 49 + , gsettings_(g_settings_new(SETTING_NAME.c_str()))
> 50 + , integrated_(false)

Well, I don't like this syntax with prefixed commas... But if you all prefer that...

> setting_changed_.Connect(gsettings_, "changed::menu-mode", [&] (GSettings*,
> gchar* key) {
> 53 + CheckSettingValue();
> 54 + });
>
> If you're not using the argument gchar* key, please use an unnamed variable
> too...

Right, I forgot to remove it (at the beginning I used the key).

> g_idle_add_full (G_PRIORITY_DEFAULT, (GSourceFunc) send_show_entry, data,
> NULL);
> g_variant_get (parameters, "(s(iiuu))", &entry_name, &geo.x, &geo.y,
> &geo.width, &geo.height);
>
> Remove the whitespace between the function name and the bracket.

Damned you! :)

> AppmenuIndicator *appmenu = dynamic_cast<AppmenuIndicator*>(indicator.get());
>
> :(

We don't have any way. But it's totally safe: we do two checks, first IsAppmenu(), then this one.

> 751 - * Copyright (C) 2010 Canonical Ltd
> 752 + * Copyright (C) 2012 Canonical Ltd
>
> Please do 2010-2012

Sure.

> ---
>
> 760 +#include <NuxCore/Rect.h>
>
> I think you can use a forward declaration.

I thought too... But it didn't work when I firstly compiled. Now I removed it again and it works... Magic!

> ---
>
> + bool IsVisible();
>
> Const?
>
> ---+ bool IsVisible();

That is done in lp:~3v1n0/unity/lim-panel... Here just focus on the UnityCore unity-panel-service changes.

> Btw all of these are not blocking ;)

If they are not blocking, please don't set your review as needs fixing! :)

Just one tiny thing:

1035 + " <arg type='(iiuu)' name='entry_id' />"

Doesn't look like id, could you update the name? Other than that it looks fine now.

> Just one tiny thing:
>
> 1035 + " <arg type='(iiuu)' name='entry_id' />"
>
> Doesn't look like id, could you update the name? Other than that it looks fine
> now.

Indeed! Copy&Paste issue :)

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/294/console reported an error when processing this lp:~3v1n0/unity/lim branch.
Not merging it.