[WK2][EFL]REGRESSION (r141978): ewk_view_type_check api test failing

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

SSIA. Assertion.
ASSERTION FAILED: evasObject && isViewEvasObject(evasObject)
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/UIProcess/API/efl/EwkView.cpp(1393) : EwkView* toEwkView(const Evas_Object*)
1   0x2aac5162c073 toEwkView(_Evas_Object const*)
2   0x2aac5164b8c1 ewk_view_context_get
3   0x448cf3 EWK2UnitTestBase_ewk_view_type_check_Test::TestBody()
4   0x2aac52a49fdd testing::Test::Run()
5   0x2aac52a4a601 testing::internal::TestInfoImpl::Run()
6   0x2aac52a4ab72 testing::TestCase::Run()
7   0x2aac52a4f53e testing::internal::UnitTestImpl::RunAllTests()
8   0x2aac52a4e2bc testing::UnitTest::Run()

[Mikhail Pozdnyakov]

Created attachment 186823 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 186823 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186823&action=review

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
> +        if (Ewk_View_Smart_Data* smartData = static_cast<Ewk_View_Smart_Data*>(evas_object_smart_data_get(evasObject)))
> +            return smartData->priv;

Is that if needed? I don't believe this will return null for an invalid cast, unlike qobject_cast or similar

> Source/WebKit2/UIProcess/API/efl/EwkView.h:-319
>  EwkView* toEwkView(const Evas_Object*);
> -EwkView* toEwkView(const Ewk_View_Smart_Data* smartData);

You removed the smartData version. That deserves a comment in the changelog. It is only used internally?

[Mikhail Pozdnyakov]

(In reply to comment #2)
> (From update of attachment 186823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186823&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
> > +        if (Ewk_View_Smart_Data* smartData = static_cast<Ewk_View_Smart_Data*>(evas_object_smart_data_get(evasObject)))
> > +            return smartData->priv;
> 
> Is that if needed? I don't believe this will return null for an invalid cast, unlike qobject_cast or similar

well, evas_object_smart_data_get itself can return null:
"A pointer to data stored using evas_object_smart_data_set(), or NULL, if none has been set."

> 
> > Source/WebKit2/UIProcess/API/efl/EwkView.h:-319
> >  EwkView* toEwkView(const Evas_Object*);
> > -EwkView* toEwkView(const Ewk_View_Smart_Data* smartData);
> 
> You removed the smartData version. That deserves a comment in the changelog. It is only used internally?

yes, it's used only within EwkView. I'll add the comment.

[Kenneth Rohde Christiansen]

Comment on attachment 186823 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186823&action=review

>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
>>> +            return smartData->priv;
>> 
>> Is that if needed? I don't believe this will return null for an invalid cast, unlike qobject_cast or similar
> 
> well, evas_object_smart_data_get itself can return null:
> "A pointer to data stored using evas_object_smart_data_set(), or NULL, if none has been set."

But it cannot really do that if it passes the isViewEvasObject test... you could assert for it though.

[Mikhail Pozdnyakov]

Created attachment 186835 [details]
patch v2

Updated change log.

[Mikhail Pozdnyakov]

(In reply to comment #4)
> (From update of attachment 186823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186823&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
> >>> +            return smartData->priv;
> >> 
> >> Is that if needed? I don't believe this will return null for an invalid cast, unlike qobject_cast or similar
> > 
> > well, evas_object_smart_data_get itself can return null:
> > "A pointer to data stored using evas_object_smart_data_set(), or NULL, if none has been set."
> 
> But it cannot really do that if it passes the isViewEvasObject test... you could assert for it though.

indeed, re-uploading..

[Mikhail Pozdnyakov]

(In reply to comment #6)
> (In reply to comment #4)
> > (From update of attachment 186823 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=186823&action=review
> > 
> > >>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1407
> > >>> +            return smartData->priv;
> > >> 
> > >> Is that if needed? I don't believe this will return null for an invalid cast, unlike qobject_cast or similar
> > > 
> > > well, evas_object_smart_data_get itself can return null:
> > > "A pointer to data stored using evas_object_smart_data_set(), or NULL, if none has been set."
> > 
> > But it cannot really do that if it passes the isViewEvasObject test... you could assert for it though.
> 
> indeed, re-uploading..

well, not actually as isViewEvasObject() checks that evas object belongs to a certain Smart class, but does not make sure that the given evas object has smart data set. Putting r? again

[Kenneth Rohde Christiansen]

LGTM

[Build Bot]

Comment on attachment 186835 [details]
patch v2

Attachment 186835 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16397188

[Mikhail Pozdnyakov]

(In reply to comment #9)
> (From update of attachment 186835 [details])
> Attachment 186835 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/16397188

Some problems on win-ews. Unrelated to the patch.

[Mikhail Pozdnyakov]

Could please anyone from WK2 owners list review this (sign it off for WK2)?

[Chris Dumez]

Comment on attachment 186835 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=186835&action=review

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:125
> +static inline EwkView* toEwkView(const Ewk_View_Smart_Data* smartData)

Why did this function get moved?

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1405
> +    if (evasObject && isViewEvasObject(evasObject)) {

Why don't we use EINA safety checks? instead of if checks? It is more common in the EFL API implementation and it would print out warnings.

[Chris Dumez]

Comment on attachment 186835 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=186835&action=review

> Source/WebKit2/UIProcess/API/efl/EwkView.h:321
> +EwkView* toEwkViewChecked(const Evas_Object*);

If this is for public EFL API, then why is it in EwkView? I think such checks should be done on ewk_view side, shouldn't they?

[Mikhail Pozdnyakov]

Comment on attachment 186835 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=186835&action=review

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:125
>> +static inline EwkView* toEwkView(const Ewk_View_Smart_Data* smartData)
> 
> Why did this function get moved?

because it's not used outside, no need to export.

>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1405
>> +    if (evasObject && isViewEvasObject(evasObject)) {
> 
> Why don't we use EINA safety checks? instead of if checks? It is more common in the EFL API implementation and it would print out warnings.

to keep code more compact, messages will be printed anyway from EWK_VIEW_IMPL_GET_OR_RETURN.
BTW, to my mind it's better to keep consistency with toEwkView() which does not emit "spank, spank, spank" to the client..

>> Source/WebKit2/UIProcess/API/efl/EwkView.h:321
>> +EwkView* toEwkViewChecked(const Evas_Object*);
> 
> If this is for public EFL API, then why is it in EwkView? I think such checks should be done on ewk_view side, shouldn't they?

Think this place is better - similar functions are in the same place. In ewk_view I would leave only wrappers for EwkView method calls.

[Chris Dumez]

Comment on attachment 186835 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=186835&action=review

>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:125
>>> +static inline EwkView* toEwkView(const Ewk_View_Smart_Data* smartData)
>> 
>> Why did this function get moved?
> 
> because it's not used outside, no need to export.

I see, makes sense.

>>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1405
>>> +    if (evasObject && isViewEvasObject(evasObject)) {
>> 
>> Why don't we use EINA safety checks? instead of if checks? It is more common in the EFL API implementation and it would print out warnings.
> 
> to keep code more compact, messages will be printed anyway from EWK_VIEW_IMPL_GET_OR_RETURN.
> BTW, to my mind it's better to keep consistency with toEwkView() which does not emit "spank, spank, spank" to the client..

This is called only in our public EFL API implementation. We want to emit warnings on bad arguments. Sure, EWK_VIEW_IMPL_GET_OR_RETURN will print a warning but we won't know which check has failed: Was the argument NULL? Was the argument not a view object? Was the smart data missing?

>>> Source/WebKit2/UIProcess/API/efl/EwkView.h:321
>>> +EwkView* toEwkViewChecked(const Evas_Object*);
>> 
>> If this is for public EFL API, then why is it in EwkView? I think such checks should be done on ewk_view side, shouldn't they?
> 
> Think this place is better - similar functions are in the same place. In ewk_view I would leave only wrappers for EwkView method calls.

This function is used only in ewk_view.cpp and is used only for checking arguments to our EFL C API. I still think it should be in ewk_view.cpp instead of EwkView (which is private implementation). We do not want people to start using this function is EwkView.cpp as it does not assert.

[Mikhail Pozdnyakov]

 
> >>> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1405
> >>> +    if (evasObject && isViewEvasObject(evasObject)) {
> >> 
> >> Why don't we use EINA safety checks? instead of if checks? It is more common in the EFL API implementation and it would print out warnings.
> > 
> > to keep code more compact, messages will be printed anyway from EWK_VIEW_IMPL_GET_OR_RETURN.
> > BTW, to my mind it's better to keep consistency with toEwkView() which does not emit "spank, spank, spank" to the client..
> 
> This is called only in our public EFL API implementation. We want to emit warnings on bad arguments. Sure, EWK_VIEW_IMPL_GET_OR_RETURN will print a warning but we won't know which check has failed: Was the argument NULL? Was the argument not a view object? Was the smart data missing?
> 
> >>> Source/WebKit2/UIProcess/API/efl/EwkView.h:321
> >>> +EwkView* toEwkViewChecked(const Evas_Object*);
> >> 
> >> If this is for public EFL API, then why is it in EwkView? I think such checks should be done on ewk_view side, shouldn't they?
> > 
> > Think this place is better - similar functions are in the same place. In ewk_view I would leave only wrappers for EwkView method calls.
> 
> This function is used only in ewk_view.cpp and is used only for checking arguments to our EFL C API. I still think it should be in ewk_view.cpp instead of EwkView (which is private implementation). We do not want people to start using this function is EwkView.cpp as it does not assert.

ok, let's do so.

[Mikhail Pozdnyakov]

Created attachment 187354 [details]
patch v3

took comments from Chris into consideration.

[Mikhail Pozdnyakov]

Created attachment 187362 [details]
patch v4

1) isViewEvasObject can emit warnings itself, so no need to put it inside EINA safety check.
2) renamed isViewEvasObject to isEwkViewEvasObject to be consistent with other function names.

[Chris Dumez]

Comment on attachment 187362 [details]
patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=187362&action=review

LGTM. Thanks.

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:124
> +    ASSERT(smartData && smartData->priv);

nit: Wouldn't it be better with 2 assertions so that we know which one fails?

[Anders Carlsson]

Comment on attachment 187362 [details]
patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=187362&action=review

> Source/WebKit2/UIProcess/API/efl/EwkView.cpp:1365
> -    ASSERT(evasObject && isViewEvasObject(evasObject));
> +    ASSERT(evasObject && isEwkViewEvasObject(evasObject));

Same comment about the assert here.

[Mikhail Pozdnyakov]

Committed r143007: <http://trac.webkit.org/changeset/143007>