After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 399403 - borked Gdk::DragContext::get_targets()
borked Gdk::DragContext::get_targets()
Status: RESOLVED FIXED
Product: gtkmm
Classification: Bindings
Component: general
2.10.x
Other All
: Normal normal
: ---
Assigned To: gtkmm-forge
gtkmm-forge
Depends on:
Blocks:
 
 
Reported: 2007-01-22 14:55 UTC by Yannick.Guesnet
Modified: 2007-06-10 10:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gtkmm_get_targets.patch (3.85 KB, patch)
2007-02-10 13:29 UTC, Murray Cumming
none Details | Review

Description Yannick.Guesnet 2007-01-22 14:55:14 UTC
Please describe the problem:
The following code:
Glib::ArrayHandle<Glib::ustring> test() {
  std::list<Glib::ustring> liste;
  liste.push_back("test1"), liste.push_back("test2"), liste.push_back("test3");
  return liste;
}

int main(int argc, char *argv[]) {
  std::list<Glib::ustring> liste=test();
  for (std::list<Glib::ustring>::const_iterator it=liste.begin(); it!=liste.end(); it++)
    std::cout << *it << " ";
  return 0;
}

leads to the following output : test1 test2 test1
instead of : test1 test2 test3

It seems that this due to the fact that the object liste in the function test is local to the function.

Gtk::DragContext::get_targets() uses similar code and don't run correctly.

Steps to reproduce:
1.
2. 
3. 


Actual results:


Expected results:


Does this happen every time?


Other information:
Comment 1 Daniel Elstner 2007-01-22 15:16:36 UTC
Your test case is broken; it is not valid to return a Glib::ArrayHandle<> that was constructed from data with local scope.  It's a wonder that it doesn't simply crash.  If as you say Gtk::DragContext::get_targets() does that, it is broken.  Therefore I'll reassign the bug to gtkmm.  Thanks for your report.

Doh.  It's called "Handle" for a reason, really.
Comment 2 Murray Cumming 2007-01-22 15:23:34 UTC
So I guess we need to allocate copies of the items? A patch would be welcome.
Comment 3 Daniel Elstner 2007-01-22 15:40:43 UTC
I just had a look at the code, and it is indeed broken.  The proper solution would be to define new traits for conversion from GdkAtom to strings, but that would break API/ABI.  By the way, GDK_POINTER_TO_ATOM() should rather be used instead of a direct cast.  I think we should do either

a) Convert each GList element to a string, as in:
   node->data = g_atom_name(G_POINTER_TO_ATOM(node->data))
   and then return a Glib::ListHandle<Glib::ustring> with OWNERSHIP_DEEP
or
b) Define the appropriate traits struct.  Come to think of it, maybe
   it even exists already.  Dunno.

Hmm, I just realized that a) break API/ABI too, since it is a list, not an array.   That could be hacked around by creating a temporary array instead.  However, in fact it doesn't matter if we break this API/ABI since it is broken to begin with.  Nobody can possibly be using it at the moment.  So I'd vote for b).
Comment 4 Murray Cumming 2007-01-23 14:41:15 UTC
Yeah, b) sounds fine. Thanks.
Comment 5 Murray Cumming 2007-02-10 13:29:59 UTC
Created attachment 82275 [details] [review]
gtkmm_get_targets.patch

Could someone test this patch, please?
Comment 6 Daniel Elstner 2007-02-12 05:17:12 UTC
Aah, nice to see that apparently the traits struct really does exist already. The patch seemed a bit strange at first, since essentially it relies on the conversion

  static_cast<GdkAtom>(node->data); // node->data has type void*

to work, and I had trouble imagining how this could compile at all. But then I figured that GdkAtom is actually a pointer:

  typedef struct _GdkAtom *GdkAtom;

That's okay then. We're stretching the API guarantees of GTK+ a little since it isn't really public, but I think that's acceptable here. So the patch looks fine to me. However:

  * gtk/src/selectiondata.ccg: Added a commented-out alternative implementation 
  of get_targets(), using ArrayHandle_AtomString. We can not actually use this
  because it breaks API of a function that apparently does work. 

No way, Gtk::SelectionData::get_targets() can't possibly work. Unless you count accessing already freed memory as working. The bug will only be apparent if the caller actually accesses the strings.
Comment 7 Murray Cumming 2007-02-14 11:35:00 UTC
>  static_cast<GdkAtom>(node->data); // node->data has type void*
>
> to work, and I had trouble imagining how this could compile at all. But then I
> figured that GdkAtom is actually a pointer:

Presumably that's what's done in C with this GList, so it's not really our problem. If applications do this then we can too.

> No way, Gtk::SelectionData::get_targets() can't possibly work. Unless you count
> accessing already freed memory as working. The bug will only be apparent if the
> caller actually accesses the strings.

I'm not convinced. I think you're talking about this code:

    //Convert the atom to a string:
    gchar* const atom_name = gdk_atom_name(targets[i]);

    Glib::ustring target;
    if(atom_name)
      target = Glib::ScopedPtr<char>(atom_name).get(); //This frees the gchar*.

    listTargets.push_back(target);

gdk_atom_name() returns a new string, which is then copied into the ustring, and then freed by the ScopedPtr. Only the copy is put in the container and returned from the function. Or does something bad happen after that?

Comment 8 Daniel Elstner 2007-02-14 16:55:17 UTC
The container is allocated locally, and the ListHandle doesn't copy the elements but only calls c_str() on each.  And no, I wouldn't want to change that -- the handles have never been intended to replace containers.  Making a shallow copy only is perfectly fine for their use case.
Comment 9 Daniel Elstner 2007-02-14 17:38:18 UTC
> Presumably that's what's done in C with this GList, so it's not really our
> problem. If applications do this then we can too.

Applications are supposed to use GDK_POINTER_TO_ATOM() on the list elements.
Comment 10 Murray Cumming 2007-02-15 17:14:10 UTC
OK, so I dont' see a way to fix Gtk::SelectionData::get_targets() either without breaking API. It's unpleasant that our traits become part of the API, but I've never liked these intermediate types anyway.

I'm tempted to leave this for gtkmm 2.12, just in case it disturbs anything slightly. But 2.12.0 could be six months away.
Comment 11 Murray Cumming 2007-06-10 10:01:16 UTC
GTK+ and gtkmm 2.12 will be ready for GNOME 2.20, so I have committed this. I'm not very happy about it, but I guess we will find out if it disturbs people.