Bug 145924 – [EFL] Crash while opening child webview with EWK_PROCESS_MODEL_MULTIPLE_SECONDARY

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Ryuan Choi]

There are some crashes when we clicked some link that opens child window via window.open or a tag with _blank target if process model is multiple secondary.

It's because multiple secondary tries to assign new webprocess if related page is null.
In order to keep the child window in same process with opener, we should pass related page when we create WebPageProxy.

[Ryuan Choi]

Created attachment 254796 [details]
first_suggestion

[Gyuyoung Kim]

Comment on attachment 254796 [details]
first_suggestion

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

> Source/WebKit2/ChangeLog:13
> +        page is null. In order to keep the child window in same process with opener,

> "In order to keep the child window in same process with opener,"

When we run EWebKit with EWK_PROCESS_MODEL_MULTIPLE_SECONDARY though, should we keep new child window in same webprocess ? Does the "same process" mean UIProcess ?

> Source/WebKit2/UIProcess/API/efl/ewk_view.h:366
> +EAPI Evas_Object *ewk_view_smart_add_with_opener(Evas *e, Evas_Smart *smart, Evas_Object *opener);

I'm not sure if "opener" is clear name for this API.

[Ryuan Choi]

(In reply to comment #2)
> Comment on attachment 254796 [details]
> first_suggestion
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254796&action=review
> 
> > Source/WebKit2/ChangeLog:13
> > +        page is null. In order to keep the child window in same process with opener,
> 
> > "In order to keep the child window in same process with opener,"
> 
> When we run EWebKit with EWK_PROCESS_MODEL_MULTIPLE_SECONDARY though, should
> we keep new child window in same webprocess ? Does the "same process" mean
> UIProcess ?
> 
Sure, If not, we can't share the information between opener and child.
same process mean that same webprocess.

> > Source/WebKit2/UIProcess/API/efl/ewk_view.h:366
> > +EAPI Evas_Object *ewk_view_smart_add_with_opener(Evas *e, Evas_Smart *smart, Evas_Object *opener);
> 
> I'm not sure if "opener" is clear name for this API.

I agree, however I don't have good name for this.
In fact, this API is little bit ugly because we should use it only for window.open and opener means the ewk_view indicating javscript object "window.opener".

Anyway, there are too many APIs to create ewk_view.

[Gyuyoung Kim]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 254796 [details]
> > first_suggestion
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=254796&action=review
> > 
> > > Source/WebKit2/ChangeLog:13
> > > +        page is null. In order to keep the child window in same process with opener,
> > 
> > > "In order to keep the child window in same process with opener,"
> > 
> > When we run EWebKit with EWK_PROCESS_MODEL_MULTIPLE_SECONDARY though, should
> > we keep new child window in same webprocess ? Does the "same process" mean
> > UIProcess ?
> > 
> Sure, If not, we can't share the information between opener and child.
> same process mean that same webprocess.

As far as I know, EWK_PROCESS_MODEL_MULTIPLE_SECONDARY creates a new webprocess for a child window, right ? Under this understanding, I don't know below sentence well. Because EWK_PROCESS_MODEL_MULTIPLE_SECONDARY makes new webprocess for new child window. But I see that child window needs to have a reference to opener window.

> "In order to keep the child window in same process with opener,"



> > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:366
> > > +EAPI Evas_Object *ewk_view_smart_add_with_opener(Evas *e, Evas_Smart *smart, Evas_Object *opener);
> > 
> > I'm not sure if "opener" is clear name for this API.
> 
> I agree, however I don't have good name for this.
> In fact, this API is little bit ugly because we should use it only for
> window.open and opener means the ewk_view indicating javscript object
> "window.opener".
> 
> Anyway, there are too many APIs to create ewk_view.

How about ewk_view_smart_add_with_parent ? Or, if we use _opener, I think we have to add more detailed description in API.

[Ryuan Choi]

Comment on attachment 254796 [details]
first_suggestion

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

>>>> Source/WebKit2/ChangeLog:13
>>>> +        page is null. In order to keep the child window in same process with opener,
>>> 
>>> 
>> 
>> Sure, If not, we can't share the information between opener and child.
>> same process mean that same webprocess.
> 
> As far as I know, EWK_PROCESS_MODEL_MULTIPLE_SECONDARY creates a new webprocess for a child window, right ? Under this understanding, I don't know below sentence well. Because EWK_PROCESS_MODEL_MULTIPLE_SECONDARY makes new webprocess for new child window. But I see that child window needs to have a reference to opener window.

EWK_PROCESS_MODEL_MULTIPLE_SECONDARY basically creates new webprocess for the new webview.
But child webview via window.open or A link(with _blank) should be created in webprocess of opener.

>>>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:366
>>>> +EAPI Evas_Object *ewk_view_smart_add_with_opener(Evas *e, Evas_Smart *smart, Evas_Object *opener);
>>> 
>>> I'm not sure if "opener" is clear name for this API.
>> 
>> I agree, however I don't have good name for this.
>> In fact, this API is little bit ugly because we should use it only for window.open and opener means the ewk_view indicating javscript object "window.opener".
>> 
>> Anyway, there are too many APIs to create ewk_view.
> 
> How about ewk_view_smart_add_with_parent ? Or, if we use _opener, I think we have to add more detailed description in API.

ewk_view_smart_add_with_parent looks general.
I will consider mode description.

BTW, I hope that we have a chance to refactor ewk APIs like MAC port ?
Then we may need only two APIs.
ewk_view_add(Evas*);
ewk_view_smart_add(Evas*, Evas_Smart*, Ewk_View_Configuration*);

Then, we don't need Ewk_Context and Ewk_PageGroup.

[Gyuyoung Kim]

(In reply to comment #5)
> BTW, I hope that we have a chance to refactor ewk APIs like MAC port ?
> Then we may need only two APIs.
> ewk_view_add(Evas*);
> ewk_view_smart_add(Evas*, Evas_Smart*, Ewk_View_Configuration*);
> 
> Then, we don't need Ewk_Context and Ewk_PageGroup.

I agree with this change. We also need to use APIConfiguration.

[Ryuan Choi]

(In reply to comment #6)
> (In reply to comment #5)
> > BTW, I hope that we have a chance to refactor ewk APIs like MAC port ?
> > Then we may need only two APIs.
> > ewk_view_add(Evas*);
> > ewk_view_smart_add(Evas*, Evas_Smart*, Ewk_View_Configuration*);
> > 
> > Then, we don't need Ewk_Context and Ewk_PageGroup.
> 
> I agree with this change. We also need to use APIConfiguration.

OK, then I will check and write an email about it.
I will hold this issue until we decide direction.

[Ryuan Choi]

Created attachment 265111 [details]
Patch

[Ryuan Choi]

Created attachment 265115 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 265115 [details]
Patch

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

LGTM now. Thanks.

> Tools/MiniBrowser/efl/main.c:2272
> +    Ewk_Context *context = ewk_view_context_get(window->ewk_view);

It's extremely rare case ewk_view_context_get() returns null though, it would be good to add ASSERT(context);

[Gyuyoung Kim]

(In reply to comment #10)
> > Tools/MiniBrowser/efl/main.c:2272
> > +    Ewk_Context *context = ewk_view_context_get(window->ewk_view);
> 
> It's extremely rare case ewk_view_context_get() returns null though, it
> would be good to add ASSERT(context);

I missed to know this file is one of MiniBrowser. Let's land !

[WebKit Commit Bot]

Comment on attachment 265115 [details]
Patch

Clearing flags on attachment: 265115

Committed r192196: <http://trac.webkit.org/changeset/192196>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Comment on attachment 265115 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:746
> +    if (!ewk_view_title_get(s_newWindowObject), "Page1")

Did you mean if (!strcmp(ewk_view_title_get(s_newWindowObject), "Page1")) ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:758
> +    if (!ewk_view_title_get(s_newWindowObject), "Page2")

ditto

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:780
> +    if (!ewk_view_title_get(s_newWindowObject), "Page1")

ditto

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:792
> +    if (!ewk_view_title_get(s_newWindowObject), "Page2")

ditto

[Csaba Osztrogonác]

(In reply to comment #14)
rs=me for the followup fix.

[Gyuyoung Kim]

Comment on attachment 265115 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:746
>> +    if (!ewk_view_title_get(s_newWindowObject), "Page1")
> 
> Did you mean if (!strcmp(ewk_view_title_get(s_newWindowObject), "Page1")) ?

OMG. My stupid fault. :(

[Ryuan Choi]

(In reply to comment #16)
> Comment on attachment 265115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265115&action=review
> 
> >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:746
> >> +    if (!ewk_view_title_get(s_newWindowObject), "Page1")
> > 
> > Did you mean if (!strcmp(ewk_view_title_get(s_newWindowObject), "Page1")) ?
> 
> OMG. My stupid fault. :(

Sorry, I created th bug 151247 to cover this issue.

After fixed these typos, there are some bugs that first test(for window.open()) affect second test(for blank target).