Page MenuHomePhabricator

efl_ui_widget: refactor constructor
ClosedPublic

Authored by bu5hm4n on Aug 27 2019, 9:29 PM.

Details

Summary

first of all, in efl-ui we should probebly ensure that a widget is
always created in a window object. Otherwise we are looking for trouble.

Additionally, calling efl_ui_win_shared_data_get on anything else than a
window object will result in a returned NULL value.
If we are not having a widget parent, there is also not much point in
calling a API that is only defined on the widget base class, so we also
move that away

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.
bu5hm4n created this revision.Aug 27 2019, 9:29 PM
bu5hm4n requested review of this revision.Aug 27 2019, 9:29 PM

@kimcinoo can you verify that this commit does not break the applications you mentioned earlier ?

This commit ensures that the safety check is definitly called with new widgets, but not for legacy. In legacy it is only called if there is a window object. Does that sound fine to you?

We have the chance with legacy vs. new api to prevent such usages ...

Let me check with this more.
Currently I have a following another BT

(gdb) bt
#0  0xb2a4ae04 in evas_object_rectangle_init (eo_obj=0x0) at ../src/lib/evas/canvas/evas_object_rectangle.c:101
#1  _efl_canvas_rectangle_efl_object_constructor (eo_obj=0x0, class_data=<optimized out>) at ../src/lib/evas/canvas/evas_object_rectangle.c:90
#2  0xb281f1f4 in efl_constructor (obj=obj@entry=0x8000933a) at src/lib/eo/efl_object.eo.c:152
#3  0xb281c66e in _efl_add_internal_start_do (file=file@entry=0xb33bad68 "../src/lib/elementary/efl_ui_bg.c", line=line@entry=64, klass_id=<optimized out>, parent_id=parent_id@entry=0x0, 
    ref=ref@entry=0 '\000', is_fallback=is_fallback@entry=0 '\000', substitute_ctor=substitute_ctor@entry=0x0, sub_ctor_data=sub_ctor_data@entry=0x0) at ../src/lib/eo/eo.c:921
#4  0xb281d046 in _efl_add_internal_start (file=file@entry=0xb33bad68 "../src/lib/elementary/efl_ui_bg.c", line=line@entry=64, klass_id=<optimized out>, parent_id=parent_id@entry=0x0, ref=ref@entry=0 '\000', 
    is_fallback=is_fallback@entry=0 '\000') at ../src/lib/eo/eo.c:969
#5  0xb31d2664 in _efl_ui_bg_efl_object_constructor (obj=0x0, pd=0x9abf70) at ../src/lib/elementary/efl_ui_bg.c:62
#6  0xb281f1f4 in efl_constructor (obj=<optimized out>) at src/lib/eo/efl_object.eo.c:152
#7  0xb31d29d6 in _efl_ui_bg_legacy_efl_object_constructor (obj=0x800091b7, _pd=<optimized out>) at ../src/lib/elementary/efl_ui_bg.c:331
#8  0xb281f1f4 in efl_constructor (obj=obj@entry=0x800091b7) at src/lib/eo/efl_object.eo.c:152
#9  0xb281c66e in _efl_add_internal_start_do (file=0xb33bad68 "../src/lib/elementary/efl_ui_bg.c", line=line@entry=362, klass_id=<optimized out>, parent_id=parent_id@entry=0x800088a5, ref=ref@entry=0 '\000', 
    is_fallback=is_fallback@entry=0 '\000', substitute_ctor=substitute_ctor@entry=0x0, sub_ctor_data=sub_ctor_data@entry=0x0) at ../src/lib/eo/eo.c:921
#10 0xb281d046 in _efl_add_internal_start (file=<optimized out>, line=line@entry=362, klass_id=<optimized out>, parent_id=parent_id@entry=0x800088a5, ref=ref@entry=0 '\000', 
    is_fallback=is_fallback@entry=0 '\000') at ../src/lib/eo/eo.c:969
#11 0xb31d291e in elm_bg_add (parent=0x800088a5) at ../src/lib/elementary/efl_ui_bg.c:362
#12 0xb445c84e in content::RenderWidgetHostViewEfl::InitAsChild(void*) () from /usr/share/chromium-efl/lib/libchromium-impl.so
#13 0xb4467624 in content::WebContentsViewEfl::CreateViewForWidget(content::RenderWidgetHost*, bool) () from /usr/share/chromium-efl/lib/libchromium-impl.so
#14 0xb4419e2c in content::WebContentsImpl::CreateRenderWidgetHostViewForRenderManager(content::RenderViewHost*) () from /usr/share/chromium-efl/lib/libchromium-impl.so
#15 0xb4423350 in content::WebContentsImpl::CreateRenderViewForRenderManager(content::RenderViewHost*, int, int, base::UnguessableToken const&, content::FrameReplicationState const&) ()
   from /usr/share/chromium-efl/lib/libchromium-impl.so
#16 0xb42dee0c in content::RenderFrameHostManager::InitRenderView(content::RenderViewHostImpl*, content::RenderFrameProxyHost*) () from /usr/share/chromium-efl/lib/libchromium-impl.so
#17 0xb42e1566 in content::RenderFrameHostManager::Navigate(GURL const&, content::FrameNavigationEntry const&, content::NavigationEntryImpl const&, bool) ()
   from /usr/share/chromium-efl/lib/libchromium-impl.so
#18 0xb42cb798 in content::NavigatorImpl::NavigateToEntry(content::FrameTreeNode*, content::FrameNavigationEntry const&, content::NavigationEntryImpl const&, content::ReloadType, bool, bool, bool, scoped_refptr<content::ResourceRequestBody> const&) () from /usr/share/chromium-efl/lib/libchromium-impl.so
#19 0x00000000 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

And I got a following log with this change. There is a case the shared_win_data is NULL even though sd->win is not NULL.
But it seems that the BT is not related to this change. Because it occurs without this change as well.

E/EFL     ( 3769): elementary<3769> ../src/lib/elementary/efl_ui_widget.c:5811 _efl_ui_widget_efl_object_constructor() [GGGGGGGGGG] obj is widget: 1, window: 0x80000946, shared_win_data: (nil)
src/lib/elementary/efl_ui_widget.c
4831

Oops... it should be just 'return NULL' here.

Mhm that is indeed weird.
What type of widget is where [GGGGGGGGGG] obj is widget: 1, window: 0x80000946, shared_win_data: (nil) is printed in, what is its parent ?

I tried to find my way through chromium-efl to help you ... but ... i have no idea ... :(

kimcinoo added a comment.EditedAug 29 2019, 2:06 AM

I just got a clue regarding the BT.
The web framework is creating a Efl.Ui.Layout_Legacy using a Efl.Canvas.Group as a parent object.
(For your information, it seems that we are calling this Efl.Canvas.Group object as "WebView".)

The sd->window, parent_sd are NULL and efl_ui_win_shared_data_get(obj); returns NULL as well while construction the Efl.Ui.Layout_Legacy.
Yes the shared_win_data of Efl.Ui.Layout_Legacy is NULL at this point.

So when web framework adds a Efl.Ui.Bg_Legacy object using this Efl.Ui.Layout_Legacy object, the _efl_ui_widget_efl_object_constructor returns NULL .
And it seems that this causes the BT because _efl_ui_bg_efl_object_constructor is trying to add a rectangle object with NULL obj in _efl_ui_bg_efl_object_constructor
Please refer to following lines of _efl_ui_bg_efl_object_constructor.

EOLIAN static Eo *                                                                 
_efl_ui_bg_efl_object_constructor(Eo *obj, Efl_Ui_Bg_Data *pd)                     
{                                                                                  
   ...                                                                             
   obj = efl_constructor(efl_super(obj, MY_CLASS));                             // efl_constructor returns NULL
                                                                                
   ...
        pd->rect = efl_add(EFL_CANVAS_RECTANGLE_CLASS, obj,             // obj is NULL        
                           efl_gfx_color_set(efl_added, 0, 0, 0, 0),            
                           efl_content_set(efl_part(obj, "efl.rectangle"), efl_added));
}

The following condition could avoid the BT.

if (!elm_widget_is_legacy(obj) && sd->window && parent_is_widget)                                     
  EINA_SAFETY_ON_NULL_RETURN_VAL(sd->shared_win_data, NULL);

This condition would be not a correct solution.

Wait, Efl.Ui.Layout_Legacy returns true from elm_widget_is_legacy so in my patch that condition would not be executed, as sd->window is NULL ? I would like to enforce that in the new API we always need to have window object as root node. (I think i need to remove the sd->window from the if condition.)

kimcinoo added a comment.EditedAug 29 2019, 3:24 AM

Should _efl_ui_widget_efl_object_constructor return NULL?
How about adding a line to give ERR message for both legacy and new API instead of returning NULL?

For example:
ERR("TUT widget tree tree is broken!!! The parent (%p, %s) is incorrect. Please take correct parent object.", parent, efl_class_name_get(efl_class_get(parent)));

History has shown wonderfully, that app developers give a damn about this error, infact we are in this situation because this error is ignored... I want that to be a hard error in efl-api, or we need to consider that a valid usecase, which will mean a lot of changes and we would need a lot more tests for that.

bu5hm4n updated this revision to Diff 24597.Aug 29 2019, 3:52 AM
bu5hm4n edited the summary of this revision. (Show Details)

always error when not in legacy

Is this here working for you after the latest change?

I think that this will work.
If it is not allowed shared_win_data to be NULL when not in legacy, then how about CRI message or abort here because the returning NULL has potential danger making a crash somewhere else as you know.

kimcinoo added inline comments.Aug 29 2019, 6:54 AM
src/lib/elementary/efl_ui_widget.c
4814

It seems that the line to return NULL has to be here because from 4818 to 4826 lines are not necessary.

This change does not cause crash issue on legacy world.
But it seems that this could make unexpected result because _widget_add_sua will not work.

src/lib/elementary/efl_ui_widget.c
4797

Above two lines could be one line if(!efl_isa(parent, EFL_UI_WIDGET_CLASS)) because parent_is_widget is used only once with this change.

4807

Following will not work if parent is not widget.

'_efl_ui_widget_widget_sub_object_add' -> _widget_add_sub

This change does not cause crash issue on legacy world.
But it seems that this could make unexpected result because _widget_add_sua will not work.

Right now widget_add_sub is only called when the widget is a widget, so i think this works ? (Or I do not understand what you mean correctly)

src/lib/elementary/efl_ui_widget.c
4797

Jup.

4807

but parent is a widget here, that is what the if clouse arround is ensuring.

4814

We can also do that, the object will be teared down anyways after NULL is returned, so it does not have an impact ... :)

I misunderstood. The _efl_ui_widget_widget_sub_object_add will not be triggered if there is line efl_ui_widget_sub_object_add(non_widget_object, obj);

Okay, so this is now fine?

Yes but almost yes because
(1) I would like to want to know your opinion regarding this.

If it is not allowed shared_win_data to be NULL when not in legacy, then how about CRI message or abort here because the returning NULL has potential danger making a crash somewhere else as you know.

(2) I would like to want to know you do not want to remove parent_is_widget.

Yes but almost yes because
(1) I would like to want to know your opinion regarding this.

If it is not allowed shared_win_data to be NULL when not in legacy, then how about CRI message or abort here because the returning NULL has potential danger making a crash somewhere else as you know.

I general the EFL way of doing something like this is ERRing and returnung NULL. abort() here is a little bit extreme, since its basically also a "crash". The NULL value that is returned here should not cause directly a crash, as its an eo_id, which means, you will get errors later on for calling things on a NULL object, but you will not get a NULL access per se. So i would like to stay with ERR + returning NULL.

(2) I would like to want to know you do not want to remove parent_is_widget.

Jup, i will remove this >(

bu5hm4n updated this revision to Diff 24755.Sep 5 2019, 12:27 AM
bu5hm4n edited the summary of this revision. (Show Details)

remove now not really used boolean field

This change makes constructor do not call efl_ui_widget_sub_object_add if parent object is not a widget.
The efl_ui_widget_sub_object_add calls _widget_add_sub and _widget_add_sub calls evas_object_data_set(sobj, "elm-parent", obj);
So if the parent object is not a widget, then a child object does not have "elm-parent" data which is used by some widgets.

./src/lib/elementary/efl_ui_flip.c:126:   if (evas_object_data_get(sobj, "elm-parent") == obj)
./src/lib/elementary/efl_ui_widget_common.c:55:     parent = evas_object_data_get(o, "elm-parent")
./src/lib/elementary/elm_hover.c:368:   if (evas_object_data_get(sobj, "elm-parent") == obj) return EINA_TRUE;
./src/lib/elementary/els_cursor.c:435:   sobj_parent = evas_object_data_get(cur->eventarea, "elm-parent");
./src/lib/elementary/els_cursor.c:440:        sobj_parent = evas_object_data_get(sobj_parent, "elm-parent");
./src/lib/elementary/efl_ui_layout.c:581:   if (evas_object_data_get(sobj, "elm-parent") == obj) return EINA_TRUE;

So I would like you to call efl_ui_widget_sub_object_add for both case.

efl_ui_widget_sub_object_add is a function defined on Efl.Ui.Widget, if parent is not a widget, this API would just error out. We should not call it in this case. The *parent-relation-linking* via evas_object_data_set is also just done if you do something like efl_ui_widget_sub_object_add(obj, asdf) where obj is a widget and asdf is not a widget. So this would just cause an error message but nothing else :).

kimcinoo accepted this revision.Sep 10 2019, 6:39 PM

Yes you are right. The efl_ui_widget_sub_object_add could not be resolved for an object which is not a Efl.Ui.Widget class.

This revision is now accepted and ready to land.Sep 10 2019, 6:39 PM
Closed by commit rEFL63fd44ef1672: efl_ui_widget: refactor constructor (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>, committed by kimcinoo). · Explain WhySep 10 2019, 6:43 PM
This revision was automatically updated to reflect the committed changes.