Page MenuHomePhabricator

efl_ui_widget: keep backward compatibility
ClosedPublic

Authored by kimcinoo on Aug 26 2019, 8:10 PM.

Details

Summary

It was possilbe to add an image with improper parent object before aa2d94f and
56752e0. This patch makes it possible. Sure there are error messages when user
adds an image object using non widget object as below but you can see image.

ERR<28822>:elementary ../src/lib/elementary/efl_ui_widget.c:4801 _efl_ui_widget_efl_object_constructor() You passed a wrong parent parameter (0x400000007ced (null)). Elementary widget's parent should be an elementary widget.
ERR<28822>:elementary ../src/lib/elementary/efl_ui_widget.c:4803 _efl_ui_widget_efl_object_constructor() No widget data for object 0x400000007ced ((null))
ERR<28822>:eina_safety ../src/lib/elementary/efl_ui_win.c:9450 efl_ui_win_shared_data_get() safety check failed: pd == NULL
ERR<28822>:eo ../src/lib/eo/eo.c:579 _efl_object_call_resolve() in src/lib/elementary/efl_ui_widget.eo.c:256: func 'efl_ui_widget_sub_object_add' (698) could not be resolved for class 'Evas.Canvas'.

Test Plan

image = elm_image_add(evas_object_evas_get(win));

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.
kimcinoo created this revision.Aug 26 2019, 8:10 PM
kimcinoo requested review of this revision.Aug 26 2019, 8:10 PM
kimcinoo edited the test plan for this revision. (Show Details)Aug 26 2019, 8:13 PM
kimcinoo edited the test plan for this revision. (Show Details)
bu5hm4n requested changes to this revision.Aug 26 2019, 10:41 PM

Things like this is exactly what i meant with (https://phab.enlightenment.org/D9713#180690)

Independent from my review below: What drove a user to keep a piece of code that emits 5 lines of errors ? These lines are telling the user quite obviously that there is something wrong, why was that ever done?

Additionally, there are other cases in elm where pd->shared_win_data is used, can you also equip them with a NULL check? Crashes there are just waiting for someone to find them ... :)

src/lib/elementary/efl_ui_widget.c
399

We want to execute the code below if pd->shared_win_data is NULL

459

In case of pd->shared_win_data == NULL the code below must be executed.

4825

Please keep this and just execute it if the parent is a EFL_UI_WIDGET and sd->window is != NULL.

This revision now requires changes to proceed.Aug 26 2019, 10:41 PM
kimcinoo updated this revision to Diff 24521.Aug 27 2019, 3:27 AM

Updated based on comment.

Things like this is exactly what i meant with (https://phab.enlightenment.org/D9713#180690)

Independent from my review below: What drove a user to keep a piece of code that emits 5 lines of errors ? These lines are telling the user quite obviously that there is something wrong, why was that ever done?

Additionally, there are other cases in elm where pd->shared_win_data is used, can you also equip them with a NULL check? Crashes there are just waiting for someone to find them ... :)

It is not easy to change application implementation on my side.
One fine day, suddenly, well working fortunate application lost their image. I think this kind of issue is breaking behavior compatibility.

As you might know the new focus manager logic does not work on the Tizen.
I know we had pretty big suffering when we upgrade EFL. But I do not have any idea why we did not revert the new focus manager logic which did not keep backward compatibility at that time.
If you have history, please let me know. This kind of thing creates another difference.

This has nothing to do with focus nor focus-manager this is a place to share information about the state of the widget-tree. Any feature building on this or on the fact of having a window object at their top will not work.
The widgets need a window object to have several things working, which includes focus, but also includes access, cnp, dnd, rotation propagation in themes etc.. (And that is only the stuff i am recalling from memory, i am quite sure there is more)
It is also more of a general problem that applications assume that the code is correct even if there is a block of 5 Errors. If you just wanted to have a image, why not just use a evas image ?
We can proceed with this, but i am 100% sure that this will break again, there are 0 usecases in efl that use a widget without a window object, and even if there would be a usecase or test for this, i am not even sure about its relevance, why should we test for a usecase that gives you a page of errors?

bu5hm4n accepted this revision.Aug 27 2019, 6:02 AM

Neither the less, we can continue with this, just expect that this will break again, and again, and again ...

This revision is now accepted and ready to land.Aug 27 2019, 6:02 AM
Closed by commit rEFLcfc0d4866ccb: efl_ui_widget: keep backward compatibility (authored by kimcinoo, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyAug 27 2019, 6:03 AM
This revision was automatically updated to reflect the committed changes.

Thank you for accepting and closing this patch.
I would like to share more detailed history regarding this.

As you might know our team merges upstream every week.
It was my turn including @akanad and @myoungwoon last week exactly on 21st AUG.
Because we were using commit 8c1354a "efl_ui/popup: watch for hint change (safely) on popup object" which is before following @raster commits fixing crash failures.

be6555e fix e did again after crash from dnd fix
aa21bd3 elm - efl ui - dont crash if parent is not an elm widget

So we could not push the latest EFL to the product branch especially TV product.
The crash of enlightenment made the wizard application be NOT finished.

And then we could find there were above @raster commits and merged upstream EFL again on 23rd AUG. But there were image problems.
So again, we could not push the latest EFL to the product branch especially TV product which application developer does not want theirs even though there are some error messages.
Absolutely YES. Application developer has to change the incorrect usage. I want them to change their lines. I have wanted from the first.

The lost image is one of EFL migration problems could reproduce easily using the attached example.
We had other issues related to commit aa2d94f "efl_ui_widget: add a place to share data"

(1) Every Web application got a crash with following BT on 21st AUG migration

(gdb) bt
#0  _efl_ui_widget_efl_object_constructor (obj=0x80007f57, sd=0x9d4300) at ../src/lib/elementary/efl_ui_widget.c:5719
#1  0xb283e1f4 in efl_constructor (obj=<optimized out>) at src/lib/eo/efl_object.eo.c:152
#2  0xb32ac890 in _efl_ui_layout_base_efl_object_constructor (obj=0x80007f57, sd=<optimized out>) at ../src/lib/elementary/efl_ui_layout.c:2776
#3  0xb283e1f4 in efl_constructor (obj=<optimized out>) at src/lib/eo/efl_object.eo.c:152
#4  0xb32acb04 in _efl_ui_layout_efl_object_constructor (obj=0x80007f57, _pd=<optimized out>) at ../src/lib/elementary/efl_ui_layout.c:2754
#5  0xb283e1f4 in efl_constructor (obj=<optimized out>) at src/lib/eo/efl_object.eo.c:152
#6  0xb32ae3d8 in _efl_ui_layout_legacy_efl_object_constructor (obj=0x80007f57, pd=<optimized out>) at ../src/lib/elementary/efl_ui_layout.c:3126
#7  0xb283e1f4 in efl_constructor (obj=obj@entry=0x80007f57) at src/lib/eo/efl_object.eo.c:152
#8  0xb283b66e in _efl_add_internal_start_do (file=0xb3408af0 "../src/lib/elementary/efl_ui_layout.c", line=line@entry=3135, klass_id=<optimized out>, parent_id=parent_id@entry=0x80007e55, 
    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
#9  0xb283c046 in _efl_add_internal_start (file=<optimized out>, line=line@entry=3135, klass_id=<optimized out>, parent_id=parent_id@entry=0x80007e55, ref=ref@entry=0 '\000', 
    is_fallback=is_fallback@entry=0 '\000') at ../src/lib/eo/eo.c:969
#10 0xb32ae37e in elm_layout_add (parent=0x80007e55) at ../src/lib/elementary/efl_ui_layout.c:3135
#11 0xb447ff98 in content::WebContentsViewEfl::CreateView(gfx::Size const&, void*) () from /usr/share/chromium-efl/lib/libchromium-impl.so
#12 0xb4435a20 in content::WebContentsImpl::Init(content::WebContents::CreateParams const&) () from /usr/share/chromium-efl/lib/libchromium-impl.so
#13 0xb3f7b442 in EWebView::InitializeContent() () from /usr/share/chromium-efl/lib/libchromium-impl.so
#14 0xb3f7c684 in EWebView::Initialize() () from /usr/share/chromium-efl/lib/libchromium-impl.so
#15 0xb3fae4f0 in CreateWebViewAsEvasObject(Ewk_Context*, _Eo_Opaque*, _Evas_Smart*) () from /usr/share/chromium-efl/lib/libchromium-impl.so
#16 0x0051be98 in runtime::WebViewImpl::Initialize() ()
#17 0x0051c570 in runtime::WebViewImpl::WebViewImpl(runtime::WebView*, runtime::NativeWindow*, Ewk_Context*) ()
#18 0x0050db5c in runtime::WebView::WebView(runtime::NativeWindow*, Ewk_Context*) ()
#19 0x00504920 in runtime::WebApplication::Launch(std::unique_ptr<common::AppControl, std::default_delete<common::AppControl> >) ()
#20 0x004e00cc in runtime::UiRuntime::OnAppControl(app_control_s*) ()
#21 0x004dcc08 in runtime::UiRuntime::Exec(int, char**)::{lambda(app_control_s*, void*)#5}::_FUN(app_control_s*, void*) ()
#22 0xb3c15414 in __ui_app_control () from /lib/libcapi-appfw-application.so.0
#23 0xaa219862 in __on_control () from /usr/share/appcore/plugins/libappcore-ui-plugin.so
#24 0xb10c0196 in appcore_base_on_receive () from /lib/libappcore-common.so.1

(2) And again Web application got a crash with following BT on 23rd AUG migration

(gdb) bt
#0  0xb6427238 in _logical_parent_eval (state_change_to_parent=<synthetic pointer>, should=0 '\000', pd=0x11423e0, obj=0x8000ecf3) at ../src/lib/elementary/efl_ui_widget.c:543
#1  _full_eval (obj=0x8000ecf3, pd=0x11423e0) at ../src/lib/elementary/efl_ui_widget.c:612
#2  0xb642f954 in _efl_ui_widget_widget_parent_set (obj=0x8000ecf3, pd=0x11423e0, parent=0x80007fe6) at ../src/lib/elementary/efl_ui_widget.c:1672
#3  0xb641a8f6 in efl_ui_widget_parent_set (obj=obj@entry=0x8000ecf3, parent=parent@entry=0x80007fe6) at src/lib/elementary/efl_ui_widget.eo.c:212
#4  0xb6430172 in _efl_ui_widget_widget_sub_object_add (obj=0x80007fe6, sd=0x11405a8, sobj=0x8000ecf3) at ../src/lib/elementary/efl_ui_widget.c:1745
#5  0xb641af64 in efl_ui_widget_sub_object_add (obj=0x80007fe6, sub_obj=sub_obj@entry=0x8000ecf3) at src/lib/elementary/efl_ui_widget.eo.c:268
#6  0xb6426ecc in elm_widget_sub_object_add (obj=<optimized out>, sub_obj=sub_obj@entry=0x8000ecf3) at ../src/lib/elementary/efl_ui_widget_eo.legacy.c:97
#7  0xb638bd36 in _efl_ui_layout_base_efl_ui_widget_widget_sub_object_add (obj=0x80007fe6, _pd=<optimized out>, sobj=0x8000ecf3) at ../src/lib/elementary/efl_ui_layout.c:748
#8  0xb641af64 in efl_ui_widget_sub_object_add (obj=obj@entry=0x80007fe6, sub_obj=sub_obj@entry=0x8000ecf3) at src/lib/elementary/efl_ui_widget.eo.c:268
#9  0xb642e1fe in _efl_ui_widget_efl_object_constructor (obj=0x8000ecf3, sd=0x11423e0) at ../src/lib/elementary/efl_ui_widget.c:5766
#10 0xb5ad81f4 in efl_constructor (obj=<optimized out>) at src/lib/eo/efl_object.eo.c:152
#11 0xb635f722 in _efl_ui_image_efl_object_constructor (obj=0x8000ecf3, pd=0x1142528) at ../src/lib/elementary/efl_ui_image.c:884
#12 0xb5ad81f4 in efl_constructor (obj=obj@entry=0x8000ecf3) at src/lib/eo/efl_object.eo.c:152
#13 0xb5ad566e in _efl_add_internal_start_do (file=file@entry=0xb64b1cb0 "../src/lib/elementary/efl_ui_bg.c", line=line@entry=52, klass_id=<optimized out>, parent_id=parent_id@entry=0x80007fe6, 
    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
#14 0xb5ad6046 in _efl_add_internal_start (file=file@entry=0xb64b1cb0 "../src/lib/elementary/efl_ui_bg.c", line=line@entry=52, klass_id=<optimized out>, parent_id=parent_id@entry=0x80007fe6, 
    ref=ref@entry=0 '\000', is_fallback=is_fallback@entry=0 '\000') at ../src/lib/eo/eo.c:969
#15 0xb62c973a in _efl_ui_bg_efl_object_constructor (obj=0x80007fe6, pd=0x1140720) at ../src/lib/elementary/efl_ui_bg.c:50
#16 0xb5ad81f4 in efl_constructor (obj=<optimized out>) at src/lib/eo/efl_object.eo.c:152
#17 0xb62c99de in _efl_ui_bg_legacy_efl_object_constructor (obj=0x80007fe6, _pd=<optimized out>) at ../src/lib/elementary/efl_ui_bg.c:325
#18 0xb5ad81f4 in efl_constructor (obj=obj@entry=0x80007fe6) at src/lib/eo/efl_object.eo.c:152
#19 0xb5ad566e in _efl_add_internal_start_do (file=0xb64b1cb0 "../src/lib/elementary/efl_ui_bg.c", line=line@entry=356, klass_id=<optimized out>, parent_id=parent_id@entry=0x80039b8b, 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
#20 0xb5ad6046 in _efl_add_internal_start (file=<optimized out>, line=line@entry=356, klass_id=<optimized out>, parent_id=parent_id@entry=0x80039b8b, ref=ref@entry=0 '\000', 
    is_fallback=is_fallback@entry=0 '\000') at ../src/lib/eo/eo.c:969
#21 0xb62c9926 in elm_bg_add (parent=0x80039b8b) at ../src/lib/elementary/efl_ui_bg.c:356
#22 0xa4fec84e in content::RenderWidgetHostViewEfl::InitAsChild(void*) () from /usr/share/chromium-efl/lib/libchromium-impl.so

(gdb) p pd->shared_win_data 
$2 = (void *) 0x0
533 static Efl_Ui_Focus_Object*                                                     
534 _logical_parent_eval(Eo *obj EINA_UNUSED, Elm_Widget_Smart_Data *pd, Eina_Bool should, Eina_Bool *state_change_to_parent)
535 {                                                                               
536    //TIZEN_ONLY(20180607): Restore legacy focus                                 
537    if (elm_widget_is_legacy(obj))                                               
538      return NULL;                                                               
539    //                                                                           
540    Efl_Ui_Widget *parent;                                                       
541    Efl_Ui_Focus_Parent_Provider *provider;                                      
542                                                                                 
543    if (((Efl_Ui_Shared_Win_Data*)pd->shared_win_data)->custom_parent_provider)

I could find that there are only focus related things related to the shared_win_data as commit aa2d94f "efl_ui_widget: add a place to share data" message has.
Someday this shared_win_data could be used for access, dnd and so on. At that time, same nightmare will come again.
My humble opinion is that this continuing break started from the focus manager logic. How could we prevent this planed and expected nightmare?
Every member of our team dislikes to do the EFL migration these days because there is a lot of issues for more than 1 year.
If we are thinking that the latest EFL is not reliable and stable then how could application developer even trusts us.

I am deeply sorry if this commit caused so many issues. However, all i can say is, EFL-DnD worked before and after that, so as the whole testsuite and the examples that worked before also continue working as they were working before. Please see, i have no access those applications you are refering to, i do not know how they internally work etc.. So if there are issues in those applications that are repeatingly popping up, can you provide us a test-case that shows its usage of EFL? As long as we do not have the test code, we cannot verify that future commits are again breaking it.

Those commits from raster are new to be, i have no seen them before, and frankly said they are looking a bit wacky as efl_ui_win_shared_data_get will return just plain NULL, which is likely the case causing backtrace number (2).

The two optimizations i have done for now are focus specific, right. However, others can follow, that commit itself is pretty much unrelated to focus. What i wanted to say with my comment about access, dnd and so is that this code right now builds up on the assertion that there is a window object at the top, without a window object at the top, things are not going to work properly.

Summing up: I can understand your anger about that, and i am sorry. However, please see my side, i cannot do much more than test what is there, in EFL, dnd etc. worked after the inital commit. Usecases that are using widgets different to how we use it in EFL definitely would benefit from a testcase testing for their behavior, otherwise we will run into those issues again and again and again.

Err... I am sorry for making you say sorry. You do not need to feel sorry for this. I am not a quick-tempered man but a patient man, so I was not filled with anger and I am embarrassed about this. I and our team member just want to overcome this kind of difficulty wisely.
The application is not a just application. Those BTs came from a part of the web framework which is using EFL internally. I could find lines using EFL but I am not sure which object is using as a parent of the newly added object at runtime. I could update if I have any clue when I check the other crash mentioned on D9762.