Bug 216120 – [GLIB] RemoteInspectorServer is not started if WebKitWebContext is not already created

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Pablo Saavedra]

For the cases where "WEBKIT_INSPECTOR_SERVER" is defined as an envvar.


Let's think in a case where we are initializing the  UI process (ex: a WebKit launcher) and we have a code like this

```
...
webkit_user_content_filter_store_save()
webkit_web_context_new_with_website_data_manager()
...
```

(note that before `webkit_user_content_filter_store_save()` no other WebContext or WebView was initialized nor created).



When the webkit_user_content_filter_store_save() calls RemoteInspector.singleton() [WebKit!L72](https://github.com/WebKit/webkit/blame/d2c95d8ce5e68f5c81983dad44085dbf915ca52b/Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp#L72), the RemoteInspectorServer isn't initialized yet because this action is triggered only by any API method which returns a WebKitWebContext:

WebKitWebContext *	webkit_web_context_get_default ()
WebKitWebContext *	webkit_web_context_new ()
WebKitWebContext *	webkit_web_context_new_ephemeral ()
WebKitWebContext *	webkit_web_context_new_with_website_data_manager ()

... so the RemoteInspector object is going to fail trying to connect to the server: "RemoteInspector failed to connect to inspector server at: %s: %s"


The initialization of the RemoteInspectorServer is done in the WebKit::WebProcessPool::platformInitialize() method during the WebProcessPool::create(). And this, as I said, it only can be triggered from the WK API  by creating or getting a WebContext. This is because the initialization is done during the WebKit::WebProcessPool::WebProcessPool() creation.

My question:

Is it really needed the Inspector::RemoteInspectorServer::start() be called only from the WebKit::WebProcessPool::platformInitialize() or the 	 from WebKit::WebProcessPool::WebProcessPool()? Could be possible start the Inspector::RemoteInspectorServer from another place, early?

[Adrian Perez]

The issue that I had while working on bug #212002 where I was trying
to add API to remote inspect JSCContext instances is probably related:
we can have a bare JSCContext without any WebKitWebContext being created,
so this makes me think that maybe we should detach the creation of the
RemoteInspectorServer singleton from the WebKitWebContext, and instead
have some API to start it.

[Pablo Saavedra]

Created attachment 407979 [details]
patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Pablo Saavedra]

(In reply to Adrian Perez from comment #1)
> The issue that I had while working on bug #212002 where I was trying
> to add API to remote inspect JSCContext instances is probably related:
> we can have a bare JSCContext without any WebKitWebContext being created,
> so this makes me think that maybe we should detach the creation of the
> RemoteInspectorServer singleton from the WebKitWebContext, and instead
> have some API to start it.

Maybe. Based on your description, yes, it could be the case.

[Pablo Saavedra]

Pushed a tentative patch to address this. 

After a conversation with Carlos García, I decided to add a new WebKit/UIProcess/API/glib/WebKitInitialize what implements
its own webkitInitialize() method in a similar way that the shared InitializeWebKit2() but exclusive for the UIProcess/API objects. 

This WebKitInitialize is now the responsible of the initialization of the RemoteInspectorServer instead of WebProcessPoolGLib.

The WebKitInitialize is called in the initialization of any of the API objects what could be the first in to be initialized by a UI process:

WebKitCookieManager, WebKitFaviconDatabase, WebKitGeolocationManager, WebKitInputMethodContext, WebKitSettings, WebKitUserContentManager, WebKitWebContext, WebKitWebsiteDataManager, WebKitUserContentFilterStore, WebKitWebViewBase.

[Carlos Garcia Campos]

Comment on attachment 407979 [details]
patch

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

> Source/WebKit/ChangeLog:9
> +        Added a WebKit/UIProcess/API/glib/WebKitInitialize what implements
> +        its own webkitInitialize() to ensure the correct initialization
> +        of WebKit during the class init of the more relevant objects
> +        exposed by the API.

Not to ensure the correct initialization of WebKit, because that's already done by InitializeWebKit2, but to ensure the inspector server is initialized as early as possible, before other api calls that would depend on the inspector server being running.

> Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp:100
> +    webkitInitialize();

It doesn't really matter but I wonder why you added this at the beginning of class_init in some cases and the end in others.

> Source/WebKit/UIProcess/API/glib/WebKitGeolocationManager.cpp:339
> +    webkitInitialize();
> +

WebKitGeolocationManager can only be crated by a WebKitWebContext, so at this point the init has already been called for sure. The user can't create a WebKitGeolocationManager.

> Source/WebKit/UIProcess/API/glib/WebKitInitialize.cpp:67
> +    if (const char* address = g_getenv("WEBKIT_INSPECTOR_SERVER"))
> +        initializeRemoteInspectorServer(address);

webkitInitialize() is going to be called multiple times, better use std::once to ensure the inspector is only initialized once.

> Source/WebKit/UIProcess/API/glib/WebKitInputMethodContext.cpp:211
> +    webkitInitialize();

This file doesn't have a using namespace WebKit, so you need to either add it, or use WebKit:: here.

[Pablo Saavedra]

Created attachment 407983 [details]
patch

[Pablo Saavedra]

(In reply to Carlos Garcia Campos from comment #6)
> Comment on attachment 407979 [details]

> Not to ensure the correct initialization of WebKit, because that's already
> done by InitializeWebKit2, but to ensure the inspector server is initialized
> as early as possible, before other api calls that would depend on the
> inspector server being running.
> 
> > Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp:100
> > +    webkitInitialize();
> 
> It doesn't really matter but I wonder why you added this at the beginning of
> class_init in some cases and the end in others.
> 

There is not an specific reason. For coherence, I will modify the patch to put it the call in the beginning of the class_init always.

[Pablo Saavedra]

Created attachment 407990 [details]
patch

[Carlos Garcia Campos]

Comment on attachment 407990 [details]
patch

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

> Source/WebKit/UIProcess/API/glib/WebKitCookieManager.cpp:122
> +    webkitInitialize();

WebKitCookieManager can only be crated by a WebKitWebsiteDataManager, so at this point the init has already been called for sure. The user can't create a WebKitCookieManager.

> Source/WebKit/UIProcess/API/glib/WebKitFaviconDatabase.cpp:77
> +    webkitInitialize();

WebKitFaviconDatabase can only be crated by a WebKitWebContext, so at this point the init has already been called for sure. The user can't create a WebKitFaviconDatabase.

> Source/WebKit/UIProcess/API/glib/WebKitInitialize.cpp:35
> +#if !PLATFORM(COCOA)

You don't need this, cocoa based ports never build this file.

> Source/WebKit/UIProcess/API/glib/WebKitInitialize.h:32
> +};

Trailing ; is not needed here.

[Pablo Saavedra]

Created attachment 408171 [details]
patch

[Pablo Saavedra]

I've updated the ticket again.. @cgarcia thanks for your comments.

[Pablo Saavedra]

Created attachment 408178 [details]
patch

[Carlos Garcia Campos]

Comment on attachment 408178 [details]
patch

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

> Source/WebKit/ChangeLog:22
> +        Reviewed by NOBODY (OOPS!).

This usually goes before he description.

[Pablo Saavedra]

Created attachment 408192 [details]
patch

[Pablo Saavedra]

(In reply to Carlos Garcia Campos from comment #14)
> Comment on attachment 408178 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408178&action=review
> 
> > Source/WebKit/ChangeLog:22
> > +        Reviewed by NOBODY (OOPS!).
> 
> This usually goes before he description.

OK. Let's do this change too.

[EWS]

Committed r266718: <https://trac.webkit.org/changeset/266718>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408192 [details].