Bug 99594 – [EFL][WK2] Refactor Ewk_Context

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

[Mikhail Pozdnyakov]

Ewk_Context should be refactored accordingly to approach from bug#99321

[Mikhail Pozdnyakov]

Created attachment 169885 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 169885 [details]
patch

r=me but pls have someone else look it over

[Chris Dumez]

Comment on attachment 169885 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:117
> +PassRefPtr<Ewk_Context> Ewk_Context::create(const char* injectedBundlePath)

It would be nicer to pass a String here as this is a C++ factory method and we convert it to a String anyway.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:130
> +Ewk_Context* Ewk_Context::defaultContext()

Why don't we return a PassRefPtr here to promote the use of smart pointers?

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:161
> +bool Ewk_Context::registerURLScheme(const char* scheme, Ewk_Url_Scheme_Request_Cb callback, void *userData)

Star on wrong side.
Would be nice if scheme would be a String.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:169
> +    WKSoupRequestManagerRegisterURIScheme(m_requestManager.get(), wkScheme.get());

Let's use C++ API here instead of the WK C API.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:181
> +void Ewk_Context::addVisitedLink(const char* visitedURL)

Would be nice to pass a String here and use C++ API inside instead of WK C API.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:304
> +    return Ewk_Context::create(adoptWK(WKContextCreate()).get()).leakRef();

What about adding a factory method to Ewk_Context which does not take any argument?

> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:48
> +    static PassRefPtr<Ewk_Context> create(const char* injectedBundlePath);

String argument would be nicer here.

> Source/WebKit2/UIProcess/API/efl/ewk_context_private.h:50
> +    static Ewk_Context* defaultContext();

PassRefPtr for consistency?

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:902
> +Evas_Object* ewk_view_add_with_context(Evas* canvas, Ewk_Context *context)

Star on wrong side

[Mikhail Pozdnyakov]

Created attachment 169899 [details]
to be landed

Took comments from Chris into consideration

[Chris Dumez]

Comment on attachment 169899 [details]
to be landed

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:297
> +    return Ewk_Context::create(adoptWK(WKContextCreate()).get()).leakRef();

I would add a factory method without no argument for this.

[Mikhail Pozdnyakov]

(In reply to comment #5)
> (From update of attachment 169899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169899&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:297
> > +    return Ewk_Context::create(adoptWK(WKContextCreate()).get()).leakRef();
> 
> I would add a factory method without no argument for this.

agree

[Mikhail Pozdnyakov]

Created attachment 169902 [details]
to be landed

[WebKit Review Bot]

Comment on attachment 169902 [details]
to be landed

Clearing flags on attachment: 169902

Committed r132072: <http://trac.webkit.org/changeset/132072>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 100011

[Chris Dumez]

Backtrace on the bot:
crash log for WebKitTestRunner (pid 4562):
STDOUT: <empty>
STDERR: 1   0x7f93fe6aa55f
STDERR: 2   0x7f93f65c1cb0
STDERR: 3   0x7f93f7cb917b
STDERR: 4   0x7f93f7b3a9c5 WebKit::WebHistoryClient::didNavigateWithNavigationData(WebKit::WebContext*, WebKit::WebPageProxy*, WebKit::WebNavigationDataStore const&, WebKit::WebFrameProxy*)
STDERR: 5   0x7f93f7b8f51f WebKit::WebProcessProxy::didNavigateWithNavigationData(unsigned long, WebKit::WebNavigationDataStore const&, unsigned long)
STDERR: 6   0x7f93f7d0b57b void CoreIPC::callMemberFunction<WebKit::WebProcessProxy, void (WebKit::WebProcessProxy::*)(unsigned long, WebKit::WebNavigationDataStore const&, unsigned long), unsigned long, WebKit::WebNavigationDataStore, unsigned long>(CoreIPC::Arguments3<unsigned long, WebKit::WebNavigationDataStore, unsigned long> const&, WebKit::WebProcessProxy*, void (WebKit::WebProcessProxy::*)(unsigned long, WebKit::WebNavigationDataStore const&, unsigned long))
STDERR: 7   0x7f93f7d0af8b void CoreIPC::handleMessage<Messages::WebProcessProxy::DidNavigateWithNavigationData, WebKit::WebProcessProxy, void (WebKit::WebProcessProxy::*)(unsigned long, WebKit::WebNavigationDataStore const&, unsigned long)>(CoreIPC::MessageDecoder&, WebKit::WebProcessProxy*, void (WebKit::WebProcessProxy::*)(unsigned long, WebKit::WebNavigationDataStore const&, unsigned long))
STDERR: 8   0x7f93f7d0ab04 WebKit::WebProcessProxy::didReceiveWebProcessProxyMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
STDERR: 9   0x7f93f7b8e8dd WebKit::WebProcessProxy::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
STDERR: 10  0x7f93f7b156e9 WebKit::WebConnectionToWebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
STDERR: 11  0x7f93f7a8e62e CoreIPC::Connection::dispatchMessage(CoreIPC::MessageID, CoreIPC::MessageDecoder&)
STDERR: 12  0x7f93f7a8e76b CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::MessageDecoder>&)
STDERR: 13  0x7f93f7a8e907 CoreIPC::Connection::dispatchOneMessage()
STDERR: 14  0x7f93f7a98a10 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
STDERR: 15  0x7f93f7a98816 WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
STDERR: 16  0x7f93fe6dc78e WTF::Function<void ()>::operator()() const
STDERR: 17  0x7f93fae259fe WebCore::RunLoop::performWork()
STDERR: 18  0x7f93fb7e012f WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)
STDERR: 19  0x7f93f6f8d7c1
STDERR: 20  0x7f93f6f8c711
STDERR: 21  0x7f93f6f8cc57 ecore_main_loop_begin
STDERR: 22  0x42e887 WTR::TestController::platformRunUntil(bool&, double)
STDERR: 23  0x41c03a WTR::TestController::runUntil(bool&, WTR::TestController::TimeoutDuration)
STDERR: 24  0x41b5b4 WTR::TestController::resetStateToConsistentValues()
STDERR: 25  0x4211fe WTR::TestInvocation::invoke()
STDERR: 26  0x41bd7c WTR::TestController::runTest(char const*)
STDERR: 27  0x41beb5 WTR::TestController::runTestingServerLoop()
STDERR: 28  0x41bf45 WTR::TestController::run()
STDERR: 29  0x419e12 WTR::TestController::TestController(int, char const**)
STDERR: 30  0x42ea01 main
STDERR: 31  0x7f93f5cfe76d __libc_start_main
STDERR: LEAK: 1 WebPageProxy
STDERR: LEAK: 1 WebContext
STDERR: 
STDERR: (process:4569): libsoup-WARNING **: Cache flush finished despite 1 pending requests

[Chris Dumez]

Comment on attachment 169902 [details]
to be landed

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

Unrelated to the crashing but I found a leak I believe

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:292
> +    return Ewk_Context::defaultContext().leakRef();

Should be "Ewk_Context::create().get()" since we are not supposed to ref for the app here.

[Mikhail Pozdnyakov]

The problem happens because EWK_Context is created new all the time, even for the same instance of WKContext. Each new EWK_Context resets clients for 
WKContext and sets itself as user_data to all the client callbacks.

The crashing test creates the new view and deletes it (after it's closed) => corresponding EWK_Context is also deleted, but clients used by essential view's WKContext (for instance history client) still contain pointer to the deleted EWK_Context => pointer is dereferenced => BOOM!

It is a different problem actually beyond the scope of refactoring, but I'm going to include the fix for it to the same patch in order to avoid rebasing.

[Mikhail Pozdnyakov]

Created attachment 170077 [details]
patch + fix providing uniqueness of corresponding EwkContext and WKContext instances

[WebKit Review Bot]

Comment on attachment 170077 [details]
patch + fix providing uniqueness of corresponding EwkContext and WKContext instances

Clearing flags on attachment: 170077

Committed r132192: <http://trac.webkit.org/changeset/132192>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.