Bug 109038

Summary:

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

Product:

WebKit

Reporter:

Mikhail Pozdnyakov <mikhail.pozdnyakov>

Component:

WebKit EFL

Assignee:

Mikhail Pozdnyakov <mikhail.pozdnyakov>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, ap, benjamin, d-r, gyuyoung.kim, kenneth, kling, lucas.de.marchi, rakuco, sam, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

106752

Attachments:

Description	Flags
patch	none
patch v2	buildbot: commit-queue-
patch v3	none
patch v4	andersca: review+

Mikhail Pozdnyakov

Reported 2013-02-06 03:29:09 PST

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()

Attachments
patch (4.22 KB, patch) 2013-02-06 04:28 PST, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v2 (4.37 KB, patch) 2013-02-06 05:24 PST, Mikhail Pozdnyakov	buildbot: commit-queue-	Details Formatted Diff Diff
patch v3 (4.29 KB, patch) 2013-02-08 13:08 PST, Mikhail Pozdnyakov	no flags	Details Formatted Diff Diff
patch v4 (4.70 KB, patch) 2013-02-08 14:02 PST, Mikhail Pozdnyakov	andersca: review+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Mikhail Pozdnyakov

Comment 1 2013-02-06 04:28:05 PST

Created attachment 186823 [details] patch

Kenneth Rohde Christiansen

Comment 2 2013-02-06 05:14:05 PST

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

Comment 3 2013-02-06 05:19:17 PST

(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 4 2013-02-06 05:20:20 PST

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

Comment 5 2013-02-06 05:24:19 PST

Created attachment 186835 [details] patch v2 Updated change log.

Mikhail Pozdnyakov

Comment 6 2013-02-06 05:29:35 PST

(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

Comment 7 2013-02-06 05:44:29 PST

(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

Comment 8 2013-02-06 05:45:32 PST

LGTM

Build Bot

Comment 9 2013-02-06 07:17:49 PST

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

Comment 10 2013-02-06 07:23:12 PST

(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

Comment 11 2013-02-08 11:34:32 PST

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

Chris Dumez

Comment 12 2013-02-08 11:38:12 PST

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 13 2013-02-08 11:42:41 PST

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 14 2013-02-08 11:57:55 PST

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 15 2013-02-08 12:07:26 PST

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

Comment 16 2013-02-08 13:07:29 PST

> >>> 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

Comment 17 2013-02-08 13:08:59 PST

Created attachment 187354 [details] patch v3 took comments from Chris into consideration.

Mikhail Pozdnyakov

Comment 18 2013-02-08 14:02:30 PST

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 19 2013-02-08 14:14:09 PST

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 20 2013-02-15 08:40:44 PST

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

Comment 21 2013-02-15 09:09:46 PST

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

Note You need to log in before you can comment on or make changes to this bug.