Closed Bug 1360560 Opened 7 years ago Closed 7 years ago

[Wayland] - Implement XRemote client by D-Bus service

Categories

(Core Graveyard :: X-remote, enhancement)

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

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

Details

Attachments

(4 files)

remote_client.patch
10.90 KB, patch
karlt
: feedback+
Details | Diff | Splinter Review
Bug 1360560 - Implement XRemote client by D-Bus service,
59 bytes, text/x-review-board-request
jhorak
: review+
Details
Bug 1360560 - Add destructor to nsRemoteClient, "
59 bytes, text/x-review-board-request
jhorak
: review+
Details
Bug 1360560 - Use DBusRemoteService when running on Wayland and DBus is available,
59 bytes, text/x-review-board-request
glandium
: review+
Details 
XRemote does not work on Wayland so we need to implement it somehow different. D-Bus seems to be a good alternative.
This patch implements XRemote service by D-Bus. It uses the recent infrastructure (message format, etc.) but sends that over D-Bus instead of window property.

Core of the patch is XRemoteClient::DoSendDBusCommandLine() which sends prepared buffer to session dbus service at "org.mozilla.firefox.PROFILE_NAME" where is OpenURL() method implemented by D-Bus by Bug 1360566.

D-Bus service is used only when !mIsX11Display.
Attachment #8862859 - Flags: feedback?(karlt)
Comment on attachment 8862859 [details] [diff] [review]
remote_client.patch

Sounds a reasonable approach to me.  I haven't looked carefully and probably won't have time to do so.  Is this something that Jan could review?
Attachment #8862859 - Flags: feedback?(karlt) → feedback+
Why limit this to !X11?
(In reply to Mike Hommey [:glandium] from comment #3)
> Why limit this to !X11?

That's a good question and I think we will switch to D-Bus completely one day.

I limited it to !X11 now because I don't want to break/change existing setup by Wayland patches and it also produces a convenient way to run X11 and Wayland browser along in one session - remote commands are not mixed although you still need different profiles. IIUC D-Bus remote does not bring any extra benefit on X11 systems compared to Xremote - sandbox or speed up.
Comment on attachment 8919645 [details]
Bug 1360560 - Implement XRemote client by D-Bus service,

https://reviewboard.mozilla.org/r/190546/#review198600

Again I'd like to see the implemenation of dbus client in different class. Since XRemoteClient has only 3 public methods
Init, SendCommandLine and Shutdown I would create a proxy class XRemoteClientProxy (or even use the same name XRemoteClient - to mitigate changes in nsAppRunner) implemeting same interface inherited from nsRemoteClient and forwarding implementation to the XRemoteClient or new DBusRemoteClient instance, depends on preferences/requirements/server.

I wouldn't argue to have both classes (XRemoteClientProxy and DBusRemoteClient) in the XRemoteClient.cpp/h.

By doing so we will get rid of ifdefs and dbus implementation will look quite straightforward instead of scattered in the XRemoteClient.

What do you think?

::: widget/xremoteclient/XRemoteClient.cpp:666
(Diff revision 1)
>    char cwdbuf[MAX_PATH];
>    if (!getcwd(cwdbuf, MAX_PATH))
>      return NS_ERROR_UNEXPECTED;
>  
>    // the commandline property is constructed as an array of int32_t
>    // followed by a series of null-terminated strings:
>    //
>    // [argc][offsetargv0][offsetargv1...]<workingdir>\0<argv[0]>\0argv[1]...\0
>    // (offset is from the beginning of the buffer)
>  
>    static char desktopStartupPrefix[] = " DESKTOP_STARTUP_ID=";
>  
>    int32_t argvlen = strlen(cwdbuf);
>    for (int i = 0; i < argc; ++i) {
>      int32_t len = strlen(argv[i]);
>      if (i == 0 && aDesktopStartupID) {
>        len += sizeof(desktopStartupPrefix) - 1 + strlen(aDesktopStartupID);
>      }
>      argvlen += len;
>    }
>  
>    auto* buffer = (int32_t*) malloc(argvlen + argc + 1 +
>                                        sizeof(int32_t) * (argc + 1));
>    if (!buffer)
>      return NS_ERROR_OUT_OF_MEMORY;
>  
>    buffer[0] = TO_LITTLE_ENDIAN32(argc);
>  
>    auto *bufend = (char*) (buffer + argc + 1);
>  
>    bufend = estrcpy(cwdbuf, bufend);
>  
>    for (int i = 0; i < argc; ++i) {
>      buffer[i + 1] = TO_LITTLE_ENDIAN32(bufend - ((char*) buffer));
>      bufend = estrcpy(argv[i], bufend);
>      if (i == 0 && aDesktopStartupID) {
>        bufend = estrcpy(desktopStartupPrefix, bufend - 1);
>        bufend = estrcpy(aDesktopStartupID, bufend - 1);
>      }
>    }

This can be moved out of the class for sharing between XRemoteClient and DBusRemoteClient.
Attachment #8919645 - Flags: review?(jhorak) → review-
Okay. I'd like to add build time switch to build remote support by X-Remote or DBUS. It means to rename MOZ_ENABLE_XREMOTE to MOZ_ENABLE_REMOTE which can have values "x11" or "dbus". Also change /widget/xremoteclient to /widget/remoteclient and probably change bugzilla component name. Mike, do you agree?
Flags: needinfo?(mh+mozilla)
Having this be a build-time thing doesn't sound right. Surely, for builds that support wayland, you still want xremote for when running on x11...
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #9)
> Having this be a build-time thing doesn't sound right. Surely, for builds
> that support wayland, you still want xremote for when running on x11...

Okay, let's build the DBus backend automatically when DBus is available.
Comment on attachment 8924591 [details]
Bug 1360560 - Use DBusRemoteService when running on Wayland and DBus is available,

https://reviewboard.mozilla.org/r/195822/#review201008

::: toolkit/xre/nsAppRunner.cpp:1843
(Diff revision 1)
> -  XRemoteClient client;
> -  nsresult rv = client.Init();
> +  nsAutoPtr<nsRemoteClient> client;
> +
> +  if (aIsX11Display) {
> +    client = new XRemoteClient();
> +  } else {
> +#if defined(MOZ_ENABLE_DBUS)

I guess we don't want to build DBus remote for non-wayland enabled builds. I'll add that to next iteration of the patch.
Comment on attachment 8919645 [details]
Bug 1360560 - Implement XRemote client by D-Bus service,

https://reviewboard.mozilla.org/r/190546/#review201010


C/C++ static analysis found 1 defect in this patch.

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

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: widget/xremoteclient/DBusRemoteClient.cpp:122
(Diff revision 2)
> +  dbus_message_unref(msg);
> +
> +  if (!reply) {
> +    dbus_error_free(&err);
> +    return NS_ERROR_FAILURE;
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8924591 [details]
Bug 1360560 - Use DBusRemoteService when running on Wayland and DBus is available,

https://reviewboard.mozilla.org/r/195822/#review201204

So, thinking a little about this, I have a couple (related) questions:
- is there a reason, besides backwards compatibility, not to use the dbus service on x11 as well?
- why not enable both for all builds (as long as dbus is enabled at build time), and try both, not one or the other depending on the display?

::: toolkit/xre/nsAppRunner.cpp:1848
(Diff revision 1)
> +  if (!client) {
> +    return REMOTE_NOT_FOUND;
> +  }

new in infallible in libxul, client can't be null.
Attachment #8924591 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #16)
> Comment on attachment 8924591 [details]
> Bug 1360560 - Use DBusRemoteService when running on Wayland and DBus is
> available,
> 
> https://reviewboard.mozilla.org/r/195822/#review201204
> 
> So, thinking a little about this, I have a couple (related) questions:
> - is there a reason, besides backwards compatibility, not to use the dbus
> service on x11 as well?

I don't thinks so. Generally the dbus may be more reliable than x11 so it may be beneficial for X11 builds.

> - why not enable both for all builds (as long as dbus is enabled at build
> time), and try both, not one or the other depending on the display?

That practically means to try Dbus and then fallback to X-remote on X11. 

> ::: toolkit/xre/nsAppRunner.cpp:1848
> (Diff revision 1)
> > +  if (!client) {
> > +    return REMOTE_NOT_FOUND;
> > +  }
> 
> new in infallible in libxul, client can't be null.
Comment on attachment 8919645 [details]
Bug 1360560 - Implement XRemote client by D-Bus service,

https://reviewboard.mozilla.org/r/190546/#review201364

::: widget/xremoteclient/DBusRemoteClient.cpp:26
(Diff revision 2)
> +}
> +
> +DBusRemoteClient::~DBusRemoteClient()
> +{
> +  MOZ_LOG(sRemoteLm, LogLevel::Debug, ("DBusRemoteClient::~DBusRemoteClient"));
> +  if (mConnection)

No need to check for mConnection

::: widget/xremoteclient/DBusRemoteClient.cpp:53
(Diff revision 2)
> +void
> +DBusRemoteClient::Shutdown (void)
> +{
> +  MOZ_LOG(sRemoteLm, LogLevel::Debug, ("DBusRemoteClient::Shutdown"));
> +  if (mConnection) {
> +    // This connection is owned by libdbus and we don't need to close it

If mConnection does not need to be freed, why not setting it to nullptr without checking mConnection while keeping the comment?

::: widget/xremoteclient/DBusRemoteClient.cpp:84
(Diff revision 2)
> +  return rv;
> +}
> +
> +nsresult
> +DBusRemoteClient::DoSendDBusCommandLine(const char *aProgram, const char *aProfile,
> +                                        char* aBuffer, int aLength)

nit: aBuffer can also be const char*.

::: widget/xremoteclient/DBusRemoteClient.cpp:109
(Diff revision 2)
> +  }
> +
> +  // append arguments
> +  if (!dbus_message_append_args(msg, DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE,
> +                                &aBuffer, aLength, DBUS_TYPE_INVALID)) {
> +    return NS_ERROR_FAILURE;

You might leak msg by early return.
Attachment #8919645 - Flags: review?(jhorak) → review-
Comment on attachment 8924590 [details]
Bug 1360560 - Add destructor to nsRemoteClient, "

https://reviewboard.mozilla.org/r/195820/#review201388

::: commit-message-ccbc9:1
(Diff revision 1)
> +Bug 1360560 - make nsRemoteClient usable by adding destructor, r?jhorak

"Add destructor to nsRemoteClient to support creating its instances on the heap." or like that.
Comment on attachment 8924591 [details]
Bug 1360560 - Use DBusRemoteService when running on Wayland and DBus is available,

https://reviewboard.mozilla.org/r/195822/#review202114

::: toolkit/xre/nsAppRunner.cpp:1843
(Diff revision 2)
> -  XRemoteClient client;
> -  nsresult rv = client.Init();
> +  nsAutoPtr<nsRemoteClient> client;
> +
> +  if (aIsX11Display) {
> +    client = new XRemoteClient();
> +  } else {
> +#if defined(MOZ_ENABLE_DBUS) && defined(MOZ_WAYLAND)

Considering comment 17, I was expecting this wouldn't be MOZ_WAYLAND only.

Maybe also file a bug to use both in any case?
Attachment #8924591 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #23)
> Comment on attachment 8924591 [details]
> Bug 1360560 - Use DBusRemoteService when running on Wayland and DBus is
> available,
> 
> https://reviewboard.mozilla.org/r/195822/#review202114
> 
> ::: toolkit/xre/nsAppRunner.cpp:1843
> (Diff revision 2)
> > -  XRemoteClient client;
> > -  nsresult rv = client.Init();
> > +  nsAutoPtr<nsRemoteClient> client;
> > +
> > +  if (aIsX11Display) {
> > +    client = new XRemoteClient();
> > +  } else {
> > +#if defined(MOZ_ENABLE_DBUS) && defined(MOZ_WAYLAND)
> 
> Considering comment 17, I was expecting this wouldn't be MOZ_WAYLAND only.
> 
> Maybe also file a bug to use both in any case?

Thanks Mike. Yes I'm going to file another bug for that when both (client&server) DBus client lands .
(In reply to Martin Stránský [:stransky] from comment #24)
> Thanks Mike. Yes I'm going to file another bug for that when both
> (client&server) DBus client lands .

Filed as Bug 1415078
Comment on attachment 8924590 [details]
Bug 1360560 - Add destructor to nsRemoteClient, "

https://reviewboard.mozilla.org/r/195820/#review203224
Attachment #8924590 - Flags: review?(jhorak) → review+
Comment on attachment 8919645 [details]
Bug 1360560 - Implement XRemote client by D-Bus service,

https://reviewboard.mozilla.org/r/190546/#review203238

::: widget/xremoteclient/DBusRemoteClient.cpp:76
(Diff revision 3)
> +                                      commandLine, commandLineLength);
> +  *aWindowFound = NS_SUCCEEDED(rv);
> +
> +  MOZ_LOG(sRemoteLm, LogLevel::Debug, ("DoSendDBusCommandLine returning 0x%" PRIx32 "\n",
> +                                       static_cast<uint32_t>(rv)));
> +  return rv;

Don't you leak commandLine here?

::: widget/xremoteclient/DBusRemoteClient.cpp:117
(Diff revision 3)
> +  dbus_error_init(&err);
> +  DBusMessage* reply = dbus_connection_send_with_reply_and_block(mConnection,
> +                                                                 msg, -1, &err);
> +  dbus_message_unref(msg);
> +
> +  if (!reply) {

nit: could probably be better to copy this:
http://searchfox.org/mozilla-central/source/netwerk/wifi/nsWifiScannerDBus.cpp#62

::: widget/xremoteclient/DBusRemoteClient.cpp:118
(Diff revision 3)
> +  DBusMessage* reply = dbus_connection_send_with_reply_and_block(mConnection,
> +                                                                 msg, -1, &err);
> +  dbus_message_unref(msg);
> +
> +  if (!reply) {
> +    dbus_error_free(&err);

Return there with NS_ERROR_FAILURE

::: widget/xremoteclient/DBusRemoteClient.cpp:119
(Diff revision 3)
> +                                                                 msg, -1, &err);
> +  dbus_message_unref(msg);
> +
> +  if (!reply) {
> +    dbus_error_free(&err);
> +  } else {

Don't use else there, just:
dbus_message_unref(reply);
return NS_OK;

otherwise you're comparing to already freed pointer, which is probably harmless, but not much ok.
Attachment #8919645 - Flags: review?(jhorak) → review-
Comment on attachment 8919645 [details]
Bug 1360560 - Implement XRemote client by D-Bus service,

https://reviewboard.mozilla.org/r/190546/#review203314

::: widget/xremoteclient/DBusRemoteClient.cpp:117
(Diff revision 3)
> +  dbus_error_init(&err);
> +  DBusMessage* reply = dbus_connection_send_with_reply_and_block(mConnection,
> +                                                                 msg, -1, &err);
> +  dbus_message_unref(msg);
> +
> +  if (!reply) {

Done.
Comment on attachment 8924591 [details]
Bug 1360560 - Use DBusRemoteService when running on Wayland and DBus is available,

https://reviewboard.mozilla.org/r/195822/#review202114

> Considering comment 17, I was expecting this wouldn't be MOZ_WAYLAND only.
> 
> Maybe also file a bug to use both in any case?

Filed as Bug 1415078
Comment on attachment 8919645 [details]
Bug 1360560 - Implement XRemote client by D-Bus service,

https://reviewboard.mozilla.org/r/190546/#review203698

Thanks, looks good!
Attachment #8919645 - Flags: review?(jhorak) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/05f364c80abf
Implement XRemote client by D-Bus service, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/b3f3abb09c3e
Add destructor to nsRemoteClient, r=jhorak"
https://hg.mozilla.org/integration/autoland/rev/0596dafd4d06
Use DBusRemoteService when running on Wayland and DBus is available, r=glandium
Keywords: checkin-needed
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: