Bug 99594 - [EFL][WK2] Refactor Ewk_Context
Summary: [EFL][WK2] Refactor Ewk_Context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mikhail Pozdnyakov
URL:
Keywords:
Depends on: 100011
Blocks: 99321 99696 100005
  Show dependency treegraph
 
Reported: 2012-10-17 06:50 PDT by Mikhail Pozdnyakov
Modified: 2012-10-23 01:41 PDT (History)
6 users (show)

See Also:


Attachments
patch (39.45 KB, patch)
2012-10-22 05:33 PDT, Mikhail Pozdnyakov
kenneth: review+
Details | Formatted Diff | Diff
to be landed (39.21 KB, patch)
2012-10-22 06:53 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
to be landed (39.31 KB, patch)
2012-10-22 07:04 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff
patch + fix providing uniqueness of corresponding EwkContext and WKContext instances (40.16 KB, patch)
2012-10-23 01:17 PDT, Mikhail Pozdnyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Pozdnyakov 2012-10-17 06:50:00 PDT
Ewk_Context should be refactored accordingly to approach from bug#99321
Comment 1 Mikhail Pozdnyakov 2012-10-22 05:33:29 PDT
Created attachment 169885 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2012-10-22 05:42:11 PDT
Comment on attachment 169885 [details]
patch

r=me but pls have someone else look it over
Comment 3 Chris Dumez 2012-10-22 06:11:51 PDT
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
Comment 4 Mikhail Pozdnyakov 2012-10-22 06:53:56 PDT
Created attachment 169899 [details]
to be landed

Took comments from Chris into consideration
Comment 5 Chris Dumez 2012-10-22 06:57:22 PDT
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.
Comment 6 Mikhail Pozdnyakov 2012-10-22 06:58:31 PDT
(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
Comment 7 Mikhail Pozdnyakov 2012-10-22 07:04:05 PDT
Created attachment 169902 [details]
to be landed
Comment 8 WebKit Review Bot 2012-10-22 07:47:13 PDT
Comment on attachment 169902 [details]
to be landed

Clearing flags on attachment: 169902

Committed r132072: <http://trac.webkit.org/changeset/132072>
Comment 9 WebKit Review Bot 2012-10-22 07:47:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 WebKit Review Bot 2012-10-22 09:51:06 PDT
Re-opened since this is blocked by bug 100011
Comment 11 Chris Dumez 2012-10-22 10:30:29 PDT
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
Comment 12 Chris Dumez 2012-10-22 13:19:43 PDT
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.
Comment 13 Mikhail Pozdnyakov 2012-10-23 01:13:43 PDT
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.
Comment 14 Mikhail Pozdnyakov 2012-10-23 01:17:44 PDT
Created attachment 170077 [details]
patch + fix providing uniqueness of corresponding EwkContext and WKContext instances
Comment 15 WebKit Review Bot 2012-10-23 01:41:04 PDT
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>
Comment 16 WebKit Review Bot 2012-10-23 01:41:09 PDT
All reviewed patches have been landed.  Closing bug.