GNOME Bugzilla – Bug 761009
WARNING **: Could not find extension with name owner `:1.439´ when starting incognito mode from app menu
Last modified: 2016-02-08 19:55:50 UTC
When starting incognito mode from the app menu: ** (epiphany:14444): WARNING **: Could not find extension with name owner `:1.439´. ** (epiphany:14507): WARNING **: Could not find extension with name owner `:1.440´. This has been happening for years. It doesn't happen when starting from the command line with 'epiphany -i'.
Yes, it's harmless and not necessarily a bug. We could just remove the warning, for now. The thing is that we are using the session bus for the communication between UI process and web extensions, and signlas are received by all the UI processes. We should really use a private bus instead, but in the meantime, the warning is not really a problem.
FYI, I have it mostly working with a private connection (still working on a couple regressions). It's a lot more complicated that using the session bus. :(
Created attachment 319641 [details] [review] Revert "web-overview: Do not drop thumbnail update requests" This reverts commit d1784f095b392998473eb42d879f62762336ccc8. This was a great solution for the problem of delayed thumbnail updates when the web extension was using the session bus, but with a private connection it's not enough: now the thumbnail update could be dropped in the UI process before the EphyWebExtensionProxy object is created in the UI process. The next commit will include a different solution for this.
Created attachment 319642 [details] [review] about-handler: Wait for all thumbnails before loading overview The downside is there is now a brief but noticable delay where the overview displays pure white before the thumbnails are ready, before the nice beige background is applied, but it beats losing thumbnails. This delay could be longer during high disk activity.
Created attachment 319643 [details] [review] Use private D-Bus connections for the web extension Nobody except for the appropriate Epiphany UI process should ever be using the web extension's D-Bus interface. Enforce this by switching to private D-Bus connections for the web extension, instead of advertising the web extension on the session bus. Each web extension will now act as a D-Bus server, and will receive exactly one connection from the UI process. (Of course, the same UI process may be connected to any number of web extensions.) It might simpler to make the UI process the server instead, but I chose the web extension so that the side that exports the object acts as the server. This is a bit tricky as without a message bus, it's no longer possible to own or watch names, so we need careful synchronization between the web extension and the UI process. The web extension now uses a new Ready WebKit user message to inform the UI process that it's time to try connecting. Once the web extension receives the connection, it's tempted to start sending queued PageCreated signals right away, but the UI process may not be subscribed to the signals yet, so instead it waits until the UI process calls its new Hello method. Finally, since the UI process can no longer watch for the web extension's bus name to become unowned, we need a new way to trigger the destruction of the EphyWebExtensionProxy in the UI process. We do this with a new ShuttingDown signal. Note that this doesn't seem to work when the UI process quits, but it at least should prevent the EphyWebExtensionProxys from leaking during the lifetime of the browser. We also explicitly destroy the EphyWebExtensionProxy in the event of a web process crash, as that will no longer cause the bus name to be unwatched. This won't work if the web process crashes before page_created reaches the UI process, but it's a decent best effort. And it's really not supposed to crash before the page is created (well, at all...).
(In reply to Michael Catanzaro from comment #5) > Created attachment 319643 [details] [review] [review] > Use private D-Bus connections for the web extension > > Nobody except for the appropriate Epiphany UI process should ever be > using the web extension's D-Bus interface. Enforce this by switching to > private D-Bus connections for the web extension, instead of advertising > the web extension on the session bus. Each web extension will now act as > a D-Bus server, and will receive exactly one connection from the UI > process. (Of course, the same UI process may be connected to any number > of web extensions.) > > It might simpler to make the UI process the server instead, but I chose > the web extension so that the side that exports the object acts as the > server. I don't understand this decision. Why should be the server the one exporting the objects? When using a well known bus, the bus daemon is the server and clients connect to it to export their interfaces. I think making the UI process the server not only makes thing s a lot easier, but also more robust. > This is a bit tricky as without a message bus, it's no longer possible > to own or watch names, so we need careful synchronization between the > web extension and the UI process. The web extension now uses a new Ready > WebKit user message to inform the UI process that it's time to try > connecting. Once the web extension receives the connection, it's tempted > to start sending queued PageCreated signals right away, but the UI > process may not be subscribed to the signals yet, so instead it waits > until the UI process calls its new Hello method. This is very fragile. I don't think using user script messages is the best choice here either, since you have to wait until document-loaded is emitted to notify the ui process about the web extension. I think the UI process should send the server address to the web extensions as initialization user data, so that extensions can connect to the ui process right after being spawned in the init method. The Ui process will be notified about the new connection as soon as it's established via new-connection signal and it can monitor the extension connecting to the closed signal of the connection in the new-connection callback. > Finally, since the UI process can no longer watch for the web > extension's bus name to become unowned, we need a new way to trigger the > destruction of the EphyWebExtensionProxy in the UI process. We do this > with a new ShuttingDown signal. Note that this doesn't seem to work when > the UI process quits, but it at least should prevent the > EphyWebExtensionProxys from leaking during the lifetime of the browser. > We also explicitly destroy the EphyWebExtensionProxy in the event of a > web process crash, as that will no longer cause the bus name to be > unwatched. This won't work if the web process crashes before > page_created reaches the UI process, but it's a decent best effort. And > it's really not supposed to crash before the page is created (well, at > all...). Maybe I'm missing something, but it seems to me this is all very complicated only because the extensions are servers.
Created attachment 320597 [details] [review] Switch web extensions to using private D-Bus connections Instead of using the session bus. There's no point in advertising the web extension D-Bus interface as if it could be meaningfully used by anything other than the Epiphany UI process. EphyEmbedShell will run the D-Bus server, and the web extensions will connect to it.
(In reply to Carlos Garcia Campos from comment #6) > Maybe I'm missing something, but it seems to me this is all very complicated > only because the extensions are servers. The only reason I had extensions as servers is that when I first tried making the extensions clients, I couldn't get it to work, so I gave up and tried the opposite approach. It was a terrible idea; extensions as clients is way better.
Created attachment 320598 [details] [review] Switch web extensions to using private D-Bus connections Instead of using the session bus. There's no point in advertising the web extension D-Bus interface as if it could be meaningfully used by anything other than the Epiphany UI process. EphyEmbedShell will run the D-Bus server, and the web extensions will connect to it.
Created attachment 320599 [details] [review] Switch web extensions to using private D-Bus connections Instead of using the session bus. There's no point in advertising the web extension D-Bus interface as if it could be meaningfully used by anything other than the Epiphany UI process. EphyEmbedShell will run the D-Bus server, and the web extensions will connect to it.
Review of attachment 320599 [details] [review]: Yes!, this looks much better, thanks. ::: embed/ephy-embed-shell.c @@ +717,3 @@ G_APPLICATION_CLASS (ephy_embed_shell_parent_class)->shutdown (application); + g_dbus_server_stop (priv->dbus_server); You are assuming the server creation didn't fail. Check it's not null before stopping it. ::: embed/ephy-web-extension-proxy.c @@ +115,2 @@ g_error_free (error); + return; We currently unref the web extension proxy when we fail to create the dbus proxy, so that the shell is notified by the weak ref and the extension is unregistered. @@ +157,1 @@ + g_signal_connect (g_object_ref (connection), "closed", It's a bit confusing to take the connection reference to accept the connection as a parameter of g_signal_connect. I think it clearer if we keep the connection as a member of the web extension and unref it on dispose/finalize. ::: embed/web-extension/ephy-web-extension.c @@ +1362,3 @@ + g_warning ("Failed to register web extension object: %s\n", error->message); + g_error_free (error); + return; We are leaking the connection here, this function is supposed to take the ownership. Maybe it's clearer to make this function boolean, and do the extension->dbus_connection = connection; and emit delayed signals from the caller when this returns TRUE. @@ +1421,1 @@ + extension->auth_observer = g_dbus_auth_observer_new (); Do we need to keep the observer as a member? Can't we do like the server? We pass the observer to the connection, that takes a reference, so we can connect to the signal and unref the observer after g_dbus_connection_new_for_address, no?
Thanks, good review! (In reply to Carlos Garcia Campos from comment #11) > ::: embed/ephy-embed-shell.c > @@ +717,3 @@ > G_APPLICATION_CLASS (ephy_embed_shell_parent_class)->shutdown > (application); > > + g_dbus_server_stop (priv->dbus_server); > > You are assuming the server creation didn't fail. Check it's not null before > stopping it. Good catch. I also assume the server was created successfully in initialize_web_extensions; I'll make the server address a maybe type, pass NULL there if the server is NULL, and then have the web extension quit early if the address is missing. > ::: embed/ephy-web-extension-proxy.c > @@ +115,2 @@ > g_error_free (error); > + return; > > We currently unref the web extension proxy when we fail to create the dbus > proxy, so that the shell is notified by the weak ref and the extension is > unregistered. Good point. I will add this: /* Attempt to trigger connection_closed_cb, which will destroy us, and ensure that * that EphyEmbedShell will remove us from its extensions list. */ g_dbus_connection_close (web_extension->connection, web_extension->cancellable, NULL /* GAsyncReadyCallback */, NULL); I will leave the GAsyncReadyCallback parameter NULL, because it seems pointless to add that just to do error handling for an error path? > @@ +157,1 @@ > + g_signal_connect (g_object_ref (connection), "closed", > > It's a bit confusing to take the connection reference to accept the > connection as a parameter of g_signal_connect. I think it clearer if we keep > the connection as a member of the web extension and unref it on > dispose/finalize. OK. Also, while trying to fix the point above, I realized I'm leaking the GDBusConnection in the case where the proxy is not created, with no way to fix it unless I make it a member of the web extension. > ::: embed/web-extension/ephy-web-extension.c > @@ +1362,3 @@ > + g_warning ("Failed to register web extension object: %s\n", > error->message); > + g_error_free (error); > + return; > > We are leaking the connection here, this function is supposed to take the > ownership. Maybe it's clearer to make this function boolean, and do the > extension->dbus_connection = connection; and emit delayed signals from the > caller when this returns TRUE. Really good catch. I will fix it and merge this function into dbus_connection_created_cb (the caller), which I almost did regardless. > @@ +1421,1 @@ > + extension->auth_observer = g_dbus_auth_observer_new (); > > Do we need to keep the observer as a member? Can't we do like the server? We > pass the observer to the connection, that takes a reference, so we can > connect to the signal and unref the observer after > g_dbus_connection_new_for_address, no? Um... yes, I think so. I was thinking to keep it alive through the async operation, but it's not needed, GDBusConnection does take a reference, like you'd expect.
Created attachment 320631 [details] [review] Incremental patch addressing review comments So you don't have to look at the entire patch to see my changes.
Created attachment 320634 [details] [review] Switch web extensions to using private D-Bus connections Instead of using the session bus. There's no point in advertising the web extension D-Bus interface as if it could be meaningfully used by anything other than the Epiphany UI process. EphyEmbedShell will run the D-Bus server, and the web extensions will connect to it.
Review of attachment 320634 [details] [review]: Please, fix the g_error_free that is serious issue. ::: embed/ephy-web-extension-proxy.c @@ +149,3 @@ + if (!remote_peer_vanished) + g_warning ("Unexpectedly lost connection to web extension: %s", error->message); + g_error_free (error); This error is not owned by this function, you should not free it. You could simply this as if (error && !remote_peer_vanished) g_warning ("Unexpectedly lost connection to web extension: %s", error->message); ::: embed/web-extension/ephy-web-extension.c @@ -1307,3 @@ - g_dbus_connection_unregister_object (extension->dbus_connection, - extension->registration_id); - extension->registration_id = 0; So, we no longer need to unregister the object on dispose?
(In reply to Carlos Garcia Campos from comment #15) > This error is not owned by this function, you should not free it. Oops > ::: embed/web-extension/ephy-web-extension.c > @@ -1307,3 @@ > - g_dbus_connection_unregister_object (extension->dbus_connection, > - extension->registration_id); > - extension->registration_id = 0; > > So, we no longer need to unregister the object on dispose? Seems to make no difference, since there's no message bus and the connection is about to be closed.