Page MenuHomePhabricator

elm/hoversel: use a wref to accurately track internal hover object
ClosedPublic

Authored by zmike on Apr 3 2020, 8:51 AM.

Details

Summary

this pointer is never unset, which can cause errors when attempting to
access it after the hoversel has been deactivated

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zmike created this revision.Apr 3 2020, 8:51 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

zmike requested review of this revision.Apr 3 2020, 8:51 AM
bu5hm4n requested changes to this revision.Apr 5 2020, 11:03 PM
bu5hm4n added a subscriber: bu5hm4n.

I agree with the change, however, i think that you should delete the wref somewhere around line 380. Someone might have a reason to ref the hover, or there is something going wrong and the widget lifes longer than expected. The ending of hovers lifetime *after* hoversel is deleted, would crash otherwise.

This revision now requires changes to proceed.Apr 5 2020, 11:03 PM
zmike added a comment.Apr 6 2020, 7:04 AM

Hm I'm not sure that's something that needs to be worried about. The hover object is an elm subobject of the hoversel, which means it will always be destroyed during the elm widget destruction, which happens prior to the eo object destructor that would free that block of memory.

The fact that its a elm subobject only means that it gets definitly invalidated, but not neccesserily destructed, someone can hold a ref. wrefs are resolved when destruction is happening.

zmike added a comment.Apr 6 2020, 7:21 AM

Okay, but if the hover is an internal object, and there's no possible way to access it (even in the unit test I had to include the private header to access it as the internal struct member) then how could someone hold a ref?

I cannot think of a case where this does happen right now. But its something where i would go the "better safe than sorry" approach.

zmike added a comment.Apr 6 2020, 8:14 AM

I don't think that approach is relevant in cases like this; it makes the code misleading for anyone reading without maximum context, and it isn't actually helpful. By removing the wref, the widget loses the ability to null that pointer, which can then lead to accessing it after it has been destroyed during the hoversel object's destruction.

There cannot be a case where the wref is triggered after the hoversel has been fully deallocated since the hover is a fully internal object which is never exposed by callback or API. There is, in fact, a longstanding enlightenment issue regarding this, as hoversel is the only widget which breaks desklock gadgets for this exact reason.

I don't think that approach is relevant in cases like this; it makes the code misleading for anyone reading without maximum context, and it isn't actually helpful. By removing the wref, the widget loses the ability to null that pointer, which can then lead to accessing it after it has been destroyed during the hoversel object's destruction.

There cannot be a case where the wref is triggered after the hoversel has been fully deallocated since the hover is a fully internal object which is never exposed by callback or API. There is, in fact, a longstanding enlightenment issue regarding this, as hoversel is the only widget which breaks desklock gadgets for this exact reason.

The claim that the hover is never exposed is also not entirely right, over "elm_object_parent_get" you can get the hover. This has already been done with elm_fileselector.
I dont think that removing a wref after you explicitly delete a object is in any way misleading, it is just ensuring that things do not get mixed up, as the lifetime of 2 hovers could collide, or the lifetime of a hover could outlife the hoversel.

zmike added a comment.Apr 14 2020, 7:28 AM

I don't think that approach is relevant in cases like this; it makes the code misleading for anyone reading without maximum context, and it isn't actually helpful. By removing the wref, the widget loses the ability to null that pointer, which can then lead to accessing it after it has been destroyed during the hoversel object's destruction.

There cannot be a case where the wref is triggered after the hoversel has been fully deallocated since the hover is a fully internal object which is never exposed by callback or API. There is, in fact, a longstanding enlightenment issue regarding this, as hoversel is the only widget which breaks desklock gadgets for this exact reason.

The claim that the hover is never exposed is also not entirely right, over "elm_object_parent_get" you can get the hover. This has already been done with elm_fileselector.

This doesn't appear to be the case. There's no way, outside of accessing elm widget internals, to access any object in the hover object's hierarchy externally.

I dont think that removing a wref after you explicitly delete a object is in any way misleading, it is just ensuring that things do not get mixed up, as the lifetime of 2 hovers could collide, or the lifetime of a hover could outlife the hoversel.

No, this isn't the case. The hoversel explicitly will delete the hover object during its own destruction, and attempting to trigger a new hover while an old one exists will just close the old one.

bu5hm4n resigned from this revision.Apr 14 2020, 7:37 AM

Well i dont really care that much about it, if it crashes that can be quickly added.

(Btw. create hoversel items, set a icon on them, elm_object_parent_widget_get 3 times, aaaaand you have your hover element)

zmike requested review of this revision.Apr 14 2020, 8:03 AM

(Btw. create hoversel items, set a icon on them, elm_object_parent_widget_get 3 times, aaaaand you have your hover element)

That's requiring roughly the same level of elm internals knowledge as it would take to iterate the evas smart object hierarchy properly to find it, so I think my point still stands.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 16 2020, 5:37 AM
This revision was automatically updated to reflect the committed changes.