1314968 - nsWifiScannerDBus breaks with recent NetworkManager

Status: RESOLVED FIXED

(Core :: Networking, defect, P5)

Description

[Jan Alexander Steffens [:heftig]]

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

Comment 1

[Jan Alexander Steffens [:heftig]]

Er, that should have been org.freedesktop.DBus.Properties.GetAll.

Comment 2

[Jan Alexander Steffens [:heftig]]

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

Comment 3

[Jan Alexander Steffens [:heftig]]

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

Comment 4

[Jan Alexander Steffens [:heftig]]

Attachment: fix-wifi-scanner.diff

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

Comment 5

[Shian-Yow Wu [:swu]]

(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?

Comment 6

[Jan Alexander Steffens [:heftig]]

Yes, it is. I've been applying it to Arch Linux's Firefox builds since posting it.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

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.

Comment 8

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 9

[Jan Alexander Steffens [:heftig]]

Attachment: Bug 1314968 - Disentangle nsWifiScannerDBus::SendMessage.

Make a copy of the function and specialize it for each message sent.
Avoids the mess of comparing the method name to figure out what to do.

Review commit: https://reviewboard.mozilla.org/r/180688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180688/

Comment 10

[Jan Alexander Steffens [:heftig]]

Attachment: Bug 1314968 - Explicitly specify the AccessPoint interface name.

The DBus specification allows passing an empty string as the interface to the
org.freedesktop.DBus.Properties.GetAll call to get all properties, throwing away the namespace
(interface) information.

However, GDBus does not allow this. When NetworkManager moved to using GDBus, Firefox lost the
ability to retrieve access points from NetworkManager.

Since we're only interested in properties from the org.freedesktop.NetworkManager.AccessPoint
interface, name it explicitly. This works with both the old and the new NetworkManager.

Review commit: https://reviewboard.mozilla.org/r/180690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180690/

Comment 11

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8909087 [details]
Bug 1314968 - Disentangle nsWifiScannerDBus::SendMessage.

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

Thanks!

Comment 12

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8909088 [details]
Bug 1314968 - Explicitly specify the AccessPoint interface name.

https://reviewboard.mozilla.org/r/180690/#review186942

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7f603cb82f4
Disentangle nsWifiScannerDBus::SendMessage. r=kanru
https://hg.mozilla.org/integration/autoland/rev/1c1e25c461a0
Explicitly specify the AccessPoint interface name. r=kanru

Comment 14

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d7f603cb82f4
https://hg.mozilla.org/mozilla-central/rev/1c1e25c461a0