nsWifiScannerDBus breaks with recent NetworkManager

RESOLVED FIXED in Firefox 58

Status

()

P5
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: heftig, Assigned: heftig)

Tracking

49 Branch
mozilla58
All
Linux
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [necko-would-take])

Attachments

(2 attachments, 1 obsolete attachment)

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
(Assignee)

Comment 1

2 years ago
Er, that should have been org.freedesktop.DBus.Properties.GetAll.
Whiteboard: [necko-would-take]
(Assignee)

Comment 2

2 years 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
(Assignee)

Comment 3

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

Comment 4

2 years ago
Posted patch fix-wifi-scanner.diff (obsolete) — Splinter Review
Anyway, fixing up the interface name sent restores location accuracy for me.
(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)
(Assignee)

Comment 6

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

Updated

2 years 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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8807544 - Attachment is obsolete: true

Comment 11

2 years ago
mozreview-review
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 12

2 years ago
mozreview-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+
Keywords: checkin-needed
Assignee: nobody → jan.steffens

Comment 13

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d7f603cb82f4
https://hg.mozilla.org/mozilla-central/rev/1c1e25c461a0
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.