Closed Bug 1360566 Opened 7 years ago Closed 7 years ago

[Wayland] - Implement XRemote Server by D-Bus

Categories

(Core Graveyard :: X-remote, enhancement)

Product:
Core Graveyard
Component:
X-remote
Platform:
All
Linux
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

Details

Attachments

(2 files)

remote_service.patch
10.78 KB, patch
karlt
: feedback+
glandium
: feedback-
Details | Diff | Splinter Review
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
59 bytes, text/x-review-board-request
jhorak
: review+
Details 
XRemote does not work on Wayland now and needs to be disabled. We may replace the X11 backend here with DBus. This bug implements server side of XRemote DBus service.
Remote server implementation.

When running on non-X11 server it opens D-Bus service at org.mozilla.firefox.PROFILE_NAME (at nsGTKRemoteService::Connect()) and expose org.mozilla.firefox interface, /org/mozilla/Firefox/Remote object with OpenURL() method mapped to nsGTKRemoteService::OpenURL() (that's done by org.freedesktop.DBus.Introspectable object here).

A question is how to correctly implement nsGTKRemoteService::OpenURL() - unlike the X-remote we're missing XWindow and timestamp information.
Attachment #8862862 - Flags: feedback?(karlt)
Comment on attachment 8862862 [details] [diff] [review]
remote_service.patch

(In reply to Martin Stránský from comment #1)
> When running on non-X11 server it opens D-Bus service at
> org.mozilla.firefox.PROFILE_NAME (at nsGTKRemoteService::Connect()) and

PROFILE_NAME can be arbitrary, so this might cause conflicts if a firefox
service of a different name org.mozilla.firefox.SERVICE2 is required one day.
Is that likely?  Or would it just be another interface on the same service as
Remote?

> expose org.mozilla.firefox interface, /org/mozilla/Firefox/Remote object
> with OpenURL() method mapped to nsGTKRemoteService::OpenURL() (that's done
> by org.freedesktop.DBus.Introspectable object here).
> 
> A question is how to correctly implement nsGTKRemoteService::OpenURL() -
> unlike the X-remote we're missing XWindow and timestamp information.

gdk_wayland_window_focus() does make use of the timestamp, and yet I don't see
an equivalent to gdk_x11_display_get_user_time() for Wayland and 
gdk_wayland_window_set_startup_id() is a no-op, and so I don't know how to
extract an appropriate timestamp for Wayland.

Could Jan review this, please?
Attachment #8862862 - Flags: feedback?(karlt) → feedback+
Comment on attachment 8862862 [details] [diff] [review]
remote_service.patch

Review of attachment 8862862 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/remote/nsGTKRemoteService.cpp
@@ +176,5 @@
> +{
> +  HandleCommandLine(aCommandLine, nullptr, 0);
> +}
> +
> +#define MOZILLA_REMOTE_OBJECT       "/org/mozilla/Firefox/Remote"

Firefox shouldn't be hardcoded in here and anywhere in the dbus interface. Because not everything using this code is called Firefox. Think e.g. Thunderbird, and things running firefox -app.
Attachment #8862862 - Flags: feedback-
(In reply to Mike Hommey [:glandium] from comment #3)
> Comment on attachment 8862862 [details] [diff] [review]
> remote_service.patch
> 
> Review of attachment 8862862 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/remote/nsGTKRemoteService.cpp
> @@ +176,5 @@
> > +{
> > +  HandleCommandLine(aCommandLine, nullptr, 0);
> > +}
> > +
> > +#define MOZILLA_REMOTE_OBJECT       "/org/mozilla/Firefox/Remote"
> 
> Firefox shouldn't be hardcoded in here and anywhere in the dbus interface.
> Because not everything using this code is called Firefox. Think e.g.
> Thunderbird, and things running firefox -app.

Sure, no big deal here. The object is exported as child of the dbus interface object and we can use some generic name like /org/mozilla/Remote or set app name runtime.
Assignee: nobody → stransky
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,

https://reviewboard.mozilla.org/r/190596/#review196320

I rather to see remote dbus implementation in another class derived from nsXRemoteService or even nsIRemoteService.
That would mean to modify code also there: http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4638

You should also consider not using nsXRemoteService::HandleCommandLine because it's hardly working in Wayland session (see https://mail.gnome.org/archives/commits-list/2016-November/msg02837.html) and make own implementation instead.

::: toolkit/components/remote/nsGTKRemoteService.h:79
(Diff revision 1)
>    nsInterfaceHashtable<nsPtrHashKey<GtkWidget>, nsIWeakReference> mWindows;
>    GtkWidget* mServerWindow;
> +  bool       mIsDBusRemote;
> +#ifdef ENABLE_REMOTE_DBUS
> +  RefPtr<DBusConnection> mConnection;
> +  char*      mAppName;

Could you please use nsCString instead of char*? That avoids need for releasing string later.

::: toolkit/components/remote/nsGTKRemoteService.cpp:230
(Diff revision 1)
> +
> +  return DBUS_HANDLER_RESULT_HANDLED;
> +}
> +
> +DBusHandlerResult
> +nsGTKRemoteService::OpenURL(DBusMessage *msg)

Please avoid overloading of the methods, use OpenURLMethodHandler or something different from OpenURL

::: toolkit/components/remote/nsGTKRemoteService.cpp:317
(Diff revision 1)
> +  if (!mConnection)
> +    return false;
> +
> +  dbus_connection_set_exit_on_disconnect(mConnection, false);
> +
> +  if (!aProfileName || aProfileName[0] == '\0') {

Is this required? If so, I see a problem with dev-edition default profile name:
http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2609

Maybe assert could help there instead and get rid of DEFAULT_PROFILE_NAME completely.
Attachment #8919682 - Flags: review?(jhorak) → review-
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,

https://reviewboard.mozilla.org/r/190596/#review203300

::: toolkit/components/remote/nsGTKRemoteService.cpp:317
(Diff revision 1)
> +  if (!mConnection)
> +    return false;
> +
> +  dbus_connection_set_exit_on_disconnect(mConnection, false);
> +
> +  if (!aProfileName || aProfileName[0] == '\0') {

Moved to upper level.
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,

https://reviewboard.mozilla.org/r/190596/#review203310


C/C++ static analysis found 6 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: toolkit/components/remote/nsDBusRemoteService.h:24
(Diff revision 2)
> +public:
> +  // We will be a static singleton, so don't use the ordinary methods.
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIREMOTESERVICE
> +
> +  nsDBusRemoteService(nsRemoteService* aParentRemoteService)

Error: Bad implicit conversion constructor for 'nsdbusremoteservice' [clang-tidy: mozilla-implicit-constructor]

::: toolkit/components/remote/nsDBusRemoteService.h:24
(Diff revision 2)
> +public:
> +  // We will be a static singleton, so don't use the ordinary methods.
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIREMOTESERVICE
> +
> +  nsDBusRemoteService(nsRemoteService* aParentRemoteService)

Error: Bad implicit conversion constructor for 'nsdbusremoteservice' [clang-tidy: mozilla-implicit-constructor]

::: toolkit/components/remote/nsDBusRemoteService.cpp:97
(Diff revision 2)
> +  const char *message = introspect_xml.get();
> +  dbus_message_append_args(reply,
> +     DBUS_TYPE_STRING, &message,
> +     DBUS_TYPE_INVALID);
> +
> +  dbus_connection_send(mConnection, reply, NULL);

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  dbus_connection_send(mConnection, reply, NULL);
                                           ^
                                           nullptr

::: toolkit/components/remote/nsDBusRemoteService.cpp:120
(Diff revision 2)
> +  } else {
> +   mParentRemoteService->HandleCommandLine(commandLine, nullptr, 0);
> +   reply = dbus_message_new_method_return(msg);
> +  }
> +
> +  dbus_connection_send(mConnection, reply, NULL);

Warning: Use nullptr [clang-tidy: modernize-use-nullptr]

  dbus_connection_send(mConnection, reply, NULL);
                                           ^
                                           nullptr

::: toolkit/components/remote/nsGTKRemoteService.h:26
(Diff revision 2)
>  public:
> -  // We will be a static singleton, so don't use the ordinary methods.
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIREMOTESERVICE
>  
> +  nsGTKRemoteService(nsRemoteService* aParentRemoteService)

Error: Bad implicit conversion constructor for 'nsgtkremoteservice' [clang-tidy: mozilla-implicit-constructor]

::: toolkit/components/remote/nsGTKRemoteService.h:26
(Diff revision 2)
>  public:
> -  // We will be a static singleton, so don't use the ordinary methods.
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIREMOTESERVICE
>  
> +  nsGTKRemoteService(nsRemoteService* aParentRemoteService)

Error: Bad implicit conversion constructor for 'nsgtkremoteservice' [clang-tidy: mozilla-implicit-constructor]
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,

https://reviewboard.mozilla.org/r/190596/#review204442

::: toolkit/components/remote/nsDBusRemoteService.h:15
(Diff revision 2)
> +
> +#include "nsIRemoteService.h"
> +#include "mozilla/DBusHelpers.h"
> +#include "nsString.h"
> +
> +class nsRemoteService;

When nsRemoteService calls used as static, no need to add this there.

::: toolkit/components/remote/nsDBusRemoteService.h:45
(Diff revision 2)
> +
> +  // The connection is owned by DBus library
> +  RefPtr<DBusConnection>  mConnection;
> +  nsCString               mAppName;
> +  // We're owned by this nsRemoteService instance
> +  nsRemoteService*        mParentRemoteService;

Don't add nsRemoteService as member.

::: toolkit/components/remote/nsDBusRemoteService.cpp:44
(Diff revision 2)
> +    return NS_ERROR_ALREADY_INITIALIZED;
> +
> +  mAppName = aAppName;
> +  ToLowerCase(mAppName);
> +
> +  if (!Connect(aAppName, aProfileName))

maybe nit: if you are not calling Connect from another place, you can put its content there. But I'll leave up to you.

::: toolkit/components/remote/nsDBusRemoteService.cpp:61
(Diff revision 2)
> +
> +NS_IMETHODIMP
> +nsDBusRemoteService::RegisterWindow(mozIDOMWindow* aWindow)
> +{
> +  // We don't listen for property change events on DBus remote
> +  return NS_OK;

Maybe better to return NS_ERROR_NOT_IMPLEMENTED?

::: toolkit/components/remote/nsDBusRemoteService.cpp:116
(Diff revision 2)
> +      &commandLine, &length, DBUS_TYPE_INVALID) || length == 0) {
> +   nsAutoCString errorMsg;
> +   errorMsg = nsPrintfCString("org.mozilla.%s.Error", mAppName.get());
> +   reply = dbus_message_new_error(msg, errorMsg.get(), "Wrong argument");
> +  } else {
> +   mParentRemoteService->HandleCommandLine(commandLine, nullptr, 0);

Use nsRemoteService::HandleCommandLine now.

::: toolkit/components/remote/nsDBusRemoteService.cpp:198
(Diff revision 2)
> +  if (ret == -1) {
> +   dbus_connection_unref(mConnection);
> +   mConnection = nullptr;
> +   return false;
> +  }
> +
> +  nsAutoCString objectName;
> +  objectName = nsPrintfCString("/org/mozilla/%s/Remote", aAppName);
> +
> +  if (!dbus_connection_register_object_path(mConnection, objectName.get(),
> +                                           &remoteHandlersTable, this)) {
> +   dbus_connection_unref(mConnection);
> +   mConnection = nullptr;
> +   return false;
> +  }

please fix indentation. double space character instead of one.

::: toolkit/components/remote/nsDBusRemoteService.cpp:204
(Diff revision 2)
> +  nsAutoCString objectName;
> +  objectName = nsPrintfCString("/org/mozilla/%s/Remote", aAppName);

nit: can stay in one line.

::: toolkit/components/remote/nsDBusRemoteService.cpp:209
(Diff revision 2)
> +   dbus_connection_unref(mConnection);
> +   mConnection = nullptr;

Please do doublecheck the unrefing of the mConnection. In the ::Shutdown, you've mentioned it is not require to unref, but there it is done.

::: toolkit/components/remote/nsGTKRemoteService.h:32
(Diff revision 2)
> +    : mServerWindow(nullptr)
> +    , mParentRemoteService(aParentRemoteService)
> +    { }
>  
> -  nsGTKRemoteService() :
> -    mServerWindow(nullptr) { }
> +  // Used for callback calls from Gtk+
> +  static class nsGTKRemoteService* mActiveGTKRemoteService;

if static member, better to rename to sActiveGtkRemoteService

::: toolkit/components/remote/nsGTKRemoteService.h:39
(Diff revision 2)
> +  gboolean HandlePropertyChange(GtkWidget *widget,
> +                                GdkEventProperty *event,
> +                                nsIWeakReference* aThis);
> +
> +protected:
> +  virtual const char* HandleCommandLine(char* aBuffer, nsIDOMWindow* aWindow,

There should not be need for that anymore, or do I miss something?

::: toolkit/components/remote/nsGTKRemoteService.h:51
(Diff revision 2)
> -                                              uint32_t aTimestamp) override;
> -
>    nsInterfaceHashtable<nsPtrHashKey<GtkWidget>, nsIWeakReference> mWindows;
> -  GtkWidget* mServerWindow;
> +  GtkWidget*       mServerWindow;
> +  // We're owned by this nsRemoteService instance
> +  nsRemoteService* mParentRemoteService;

as above with nsDBusService.

::: toolkit/components/remote/nsGTKRemoteService.cpp:99
(Diff revision 2)
>    gtk_widget_destroy(mServerWindow);
>    mServerWindow = nullptr;
> -  return NS_OK;
> -}
>  
> -// Set desktop startup ID to the passed ID, if there is one, so that any created
> +  mActiveGTKRemoteService = nullptr;

Is it worth to move that on the very beginning of ::Shutdown() in case that early return to be sure that the mActiveGTKRemoteService won't stay initialized to some already freed pointer?

::: toolkit/components/remote/nsGTKRemoteService.cpp:105
(Diff revision 2)
> -  if (!aDesktopStartupID.IsEmpty()) {
> +  return NS_OK;
> -    toolkit->SetDesktopStartupID(aDesktopStartupID);
> -  }
> +}
>  
> -  toolkit->SetFocusTimestamp(aTimestamp);
> +const char*
> +nsGTKRemoteService::HandleCommandLine(char* aBuffer, nsIDOMWindow* aWindow,

This is probably also not required anymore.

::: toolkit/components/remote/nsRemoteService.h:25
(Diff revision 2)
> +  // We will be a static singleton, so don't use the ordinary methods.
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIREMOTESERVICE
> +  NS_DECL_NSIOBSERVER
> +
> +  void SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,

The only call I can see is from nsRemoteService, so can be private.

::: toolkit/components/remote/nsRemoteService.h:27
(Diff revision 2)
> +  NS_DECL_NSIREMOTESERVICE
> +  NS_DECL_NSIOBSERVER
> +
> +  void SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,
> +                                      uint32_t aTimestamp);
> +  const char* HandleCommandLine(const char* aBuffer, nsIDOMWindow* aWindow,

Please make a static method, this should avoid need to pass pointer to the nsRemoteService to the nsDBusRemoteService and nsGtkRemoteService.

::: toolkit/components/remote/nsRemoteService.h:30
(Diff revision 2)
> +  RefPtr<nsIRemoteService> mDBusRemoteService;
> +  RefPtr<nsIRemoteService> mGtkRemoteService;

You can use nsCOMPtr instead of RefPtr there.

::: toolkit/components/remote/nsRemoteService.cpp:37
(Diff revision 2)
> +NS_IMETHODIMP
> +nsRemoteService::Startup(const char* aAppName, const char* aProfileName)
> +{
> +#if defined(MOZ_ENABLE_DBUS)
> +    nsresult rv;
> +    mDBusRemoteService = static_cast<nsIRemoteService*>(new nsDBusRemoteService(this));

When mDBusRemoteService is nsCOMPtr the  mDBusRemoteService = new nsDBusRemoteService();
should work there.

::: toolkit/components/remote/nsRemoteService.cpp:45
(Diff revision 2)
> +        mDBusRemoteService = nullptr;
> +    }
> +#endif
> +
> +    if (GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
> +        mGtkRemoteService = static_cast<nsIRemoteService*>(new nsGTKRemoteService(this));

same as above

::: toolkit/components/remote/nsRemoteService.cpp:66
(Diff revision 2)
> +NS_IMETHODIMP
> +nsRemoteService::RegisterWindow(mozIDOMWindow* aWindow)
> +{
> +#if defined(MOZ_ENABLE_DBUS)
> +    if (mDBusRemoteService) {
> +        mDBusRemoteService->RegisterWindow(aWindow);

If calling nsDBusRemoteService::RegisterWindow actually do nothing consider removing this call completely.

::: toolkit/components/remote/nsXRemoteService.h:11
(Diff revision 2)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #ifndef NSXREMOTESERVICE_H
>  #define NSXREMOTESERVICE_H
>  
> +#include "nsRemoteService.h"

Is this a really required there?

::: toolkit/components/remote/nsXRemoteService.h:36
(Diff revision 2)
> -
> -
>  protected:
>      nsXRemoteService();
>  
> -    static bool HandleNewProperty(Window aWindowId,Display* aDisplay,
> +    bool HandleNewProperty(Window aWindowId,Display* aDisplay,

Why is it no longer possible to be a static member function?

::: toolkit/components/remote/nsXRemoteService.h:44
(Diff revision 2)
>  
>      void XRemoteBaseStartup(const char *aAppName, const char *aProfileName);
>  
>      void HandleCommandsFor(Window aWindowId);
> -    static nsXRemoteService *sRemoteImplementation;
> -private:
> +
> +    virtual const char* HandleCommandLine(char* aBuffer, nsIDOMWindow* aWindow,

Is this still required since nsRemoteService should do the job now?
Attachment #8919682 - Flags: review?(jhorak) → review-
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,

https://reviewboard.mozilla.org/r/190596/#review204838

::: toolkit/components/remote/nsDBusRemoteService.cpp:204
(Diff revision 2)
> +  nsAutoCString objectName;
> +  objectName = nsPrintfCString("/org/mozilla/%s/Remote", aAppName);

It can't due to type-cast from nsPrintfCString to nsAutoCString.
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,

https://reviewboard.mozilla.org/r/190596/#review204442

> There should not be need for that anymore, or do I miss something?

Removed completelly.
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,

https://reviewboard.mozilla.org/r/190596/#review204442

> Why is it no longer possible to be a static member function?

Made static again.
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,

https://reviewboard.mozilla.org/r/190596/#review204442

> Is it worth to move that on the very beginning of ::Shutdown() in case that early return to be sure that the mActiveGTKRemoteService won't stay initialized to some already freed pointer?

removed mActiveGTKRemoteService.
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,

https://reviewboard.mozilla.org/r/190596/#review204442

> Please do doublecheck the unrefing of the mConnection. In the ::Shutdown, you've mentioned it is not require to unref, but there it is done.

dbus_connection_unref() is called by RefPtr here so explicit call dbus_connection_unref() is redundant.
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,

https://reviewboard.mozilla.org/r/190596/#review205316

Looks good. Just check the nits, wait for the try run and I think we're done.

::: toolkit/components/remote/moz.build:17
(Diff revision 3)
>  ]
>  
>  XPIDL_MODULE = 'toolkitremote'
>  
>  SOURCES += [
>      'nsXRemoteService.cpp',

According to https://searchfox.org/mozilla-central/source/toolkit/components/moz.build the 'remote' subdir is added to the targets only when MOZ_ENABLE_XREMOTE is set (ie --without-x is not used). Is this what we want also for the nsDbusRemoteService? Can't the nsDbusRemoteService work without x support? 

Please file a new bug for this if you plan to support remote service when --without-x.

::: toolkit/components/remote/nsGTKRemoteService.h:41
(Diff revision 3)
> -
> -  virtual void SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,
> -                                              uint32_t aTimestamp) override;
> -
>    nsInterfaceHashtable<nsPtrHashKey<GtkWidget>, nsIWeakReference> mWindows;
> -  GtkWidget* mServerWindow;
> +  GtkWidget*       mServerWindow;

nit: indentation

::: toolkit/components/remote/nsGTKRemoteService.cpp:45
(Diff revision 3)
>    mServerWindow = gtk_invisible_new();
>    gtk_widget_realize(mServerWindow);
>    HandleCommandsFor(mServerWindow, nullptr);
>  
>    for (auto iter = mWindows.Iter(); !iter.Done(); iter.Next()) {
> -    HandleCommandsFor(iter.Key(), iter.UserData());
> +      HandleCommandsFor(iter.Key(), iter.UserData());

nit: indent to two spaces
Attachment #8919682 - Flags: review?(jhorak) → review+
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,

https://reviewboard.mozilla.org/r/190596/#review205316

> According to https://searchfox.org/mozilla-central/source/toolkit/components/moz.build the 'remote' subdir is added to the targets only when MOZ_ENABLE_XREMOTE is set (ie --without-x is not used). Is this what we want also for the nsDbusRemoteService? Can't the nsDbusRemoteService work without x support? 
> 
> Please file a new bug for this if you plan to support remote service when --without-x.

IMHO that would be for Bug 1306613 - right now we don't support to build Gtk+ without X11.
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/8a07d34a17b6
[Wayland] - Implement XRemote Server by D-Bus, r=jhorak
https://hg.mozilla.org/mozilla-central/rev/8a07d34a17b6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1418667
Depends on: 1418985
Depends on: 1419019
Depends on: 1419618
Depends on: 1418782
Depends on: 1418770
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: