Ewk_Context should be refactored accordingly to approach from bug#99321
Created attachment 169885 [details] patch
Comment on attachment 169885 [details] patch r=me but pls have someone else look it over
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
Created attachment 169899 [details] to be landed Took comments from Chris into consideration
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.
(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
Created attachment 169902 [details] to be landed
Comment on attachment 169902 [details] to be landed Clearing flags on attachment: 169902 Committed r132072: <http://trac.webkit.org/changeset/132072>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 100011
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 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.
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.
Created attachment 170077 [details] patch + fix providing uniqueness of corresponding EwkContext and WKContext instances
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>