nsWifiScannerDBus breaks with recent NetworkManager

UNCONFIRMED
Unassigned

Status

()

Core
Networking
P5
normal
UNCONFIRMED
a year ago
a month ago

People

(Reporter: heftig, Unassigned)

Tracking

49 Branch
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-would-take])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
When retrieving access point attributes using org.freedesktop.Properties.GetAll, the scanner supplies an empty string as the interface name. Recent NM responds with an error "No such interface".

The scanner should use "org.freedesktop.NetworkManager.AccessPoint" as the interface name.

Firefox 49.0.2
NetworkManager 1.4.2
(Reporter)

Comment 1

a year ago
Er, that should have been org.freedesktop.DBus.Properties.GetAll.
Whiteboard: [necko-would-take]
(Reporter)

Comment 2

a year ago
Actually, the D-Bus Specification [1] says:

> An empty string may be provided for the interface name; in this
> case, if there are multiple properties on an object with the
> same name, the results are undefined (picking one by according
> to an arbitrary deterministic rule, or returning an error, are
> the reasonable possibilities). 

So this could be a bug in NM or GDBus. I think providing the above interface name should work in both old and new versions of NM, but I don't know for sure.

[1]: https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-properties
(Reporter)

Comment 3

a year ago
This is a long-standing bug in GDBus: https://bugzilla.gnome.org/show_bug.cgi?id=741101
(Reporter)

Comment 4

a year ago
Created attachment 8807544 [details] [diff] [review]
fix-wifi-scanner.diff

Anyway, fixing up the interface name sent restores location accuracy for me.

Comment 5

2 months ago
(In reply to Jan Alexander Steffens from comment #4)
> Created attachment 8807544 [details] [diff] [review]
> fix-wifi-scanner.diff
> 
> Anyway, fixing up the interface name sent restores location accuracy for me.

Is the patch ready for review?

According to last review record of this part of the code, probably :kanru can review it when ready?
Flags: needinfo?(kchen)
Flags: needinfo?(jan.steffens)
(Reporter)

Comment 6

2 months ago
Yes, it is. I've been applying it to Arch Linux's Firefox builds since posting it.
Flags: needinfo?(jan.steffens)

Updated

2 months ago
Flags: needinfo?(kchen)
Attachment #8807544 - Flags: review?(kchen)
Comment on attachment 8807544 [details] [diff] [review]
fix-wifi-scanner.diff

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

Thanks for the patch! We can improve the patch's readability by giving it 8 line of context according to the patch guideline. https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_diff_and_patch_files.3F

I have one nit, otherwise the patch looks good to me.

::: c/netwerk/wifi/nsWifiScannerDBus.cpp
@@ +62,4 @@
>        return NS_ERROR_FAILURE;
>      }
>    } else if (!strcmp(aFuncCall, "GetAll")) {
> +    const char* param = "org.freedesktop.NetworkManager.AccessPoint";

nit: This code assumes function GetAll implies interface org.freedesktop.DBus.Properties, which isn't great. I would like to add a MOZ_ASSERT here to ensure aInterface is indeed that.
Attachment #8807544 - Flags: review?(kchen) → review+
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

a month ago
Attachment #8807544 - Attachment is obsolete: true
Comment on attachment 8909087 [details]
Bug 1314968 - Disentangle nsWifiScannerDBus::SendMessage.

https://reviewboard.mozilla.org/r/180688/#review186940

Thanks!
Attachment #8909087 - Flags: review?(kchen) → review+
Comment on attachment 8909088 [details]
Bug 1314968 - Explicitly specify the AccessPoint interface name.

https://reviewboard.mozilla.org/r/180690/#review186942
Attachment #8909088 - Flags: review?(kchen) → review+
You need to log in before you can comment on or make changes to this bug.