| Summary: | [EFL] Crash while opening child webview with EWK_PROCESS_MODEL_MULTIPLE_SECONDARY | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryuan Choi <ryuan.choi> | ||||||||
| Component: | WebKit EFL | Assignee: | Ryuan Choi <ryuan.choi> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | commit-queue, gyuyoung.kim, lucas.de.marchi, ossy | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 151247 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Ryuan Choi
2015-06-12 05:05:08 PDT
Created attachment 254796 [details]
first_suggestion
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. (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. (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. 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. (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. (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. Created attachment 265111 [details]
Patch
Created attachment 265115 [details]
Patch
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); (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 ! Comment on attachment 265115 [details] Patch Clearing flags on attachment: 265115 Committed r192196: <http://trac.webkit.org/changeset/192196> All reviewed patches have been landed. Closing bug. 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 (In reply to comment #14) rs=me for the followup fix. 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. :( (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). |