Closed Bug 1619094 Opened 4 years ago Closed 3 years ago

dom/webauthn/libudev-sys is third party code

Categories

(Core :: DOM: Web Authentication, defect, P1)

Product:
Core
Component:
DOM: Web Authentication
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
85 Branch
Tracking Flags:
Tracking Status
firefox85 --- fixed

People

(Reporter: Sylvestre, Assigned: Gankra)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1619094 - Properly explain our fork of libudev-sys. r?sylvestre
47 bytes, text/x-phabricator-request
Details | Review

And we aren't using the latest version, isn't in third party and don't have a checksum file

And move from 0.1.3 to 0.1.4

Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/156832378185
move dom/webauthn/libudev-sys in third_party/rust r=jcj

Not sure what is going on, I will have a look

Flags: needinfo?(sledru)

The priority flag is not set for this bug.
:jcj, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(jjones)
Flags: needinfo?(jjones)
Priority: -- → P1

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Sylvestre, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(sledru)

Still have the patch in review, I will work on it

Flags: needinfo?(sledru)

JC, to change my mind, I had a new look to this one.

Moving place requires to update libudev to 0.1.4.

Two things are a bit confusing:

  • it requires the package libudev-dev for the pkg-config task (and probably the header). It wasn't the case before.
  • it builds but fails to link
 6:52.03 ld.lld: error: undefined symbol: udev_enumerate_add_match_subsystem
 6:52.03 >>> referenced by enumerator.rs:49 (/home/sylvestre/dev/mozilla/mozilla-central.hg/third_party/rust/libudev/src/enumerator.rs:49)
 6:52.03 >>>               authenticator-abc69fe5e60fbfa2.authenticator.dn9qihlx-cgu.9.rcgu.o:(libudev::enumerator::Enumerator::match_subsystem::ha27796f8f8e35c01) in archive /home/sylvestre/dev/mozilla/mozilla-central.hg/obj-x86_64-pc-linux-gnu/x86_64-unknown-linux-gnu/debug/libgkrust.a

So, my question is simple: was it really compiled and linked before? :)

Flags: needinfo?(jjones)

I mean, it was... that is a fairly important part, and no one is complaining about it being broken on udev-using platforms.

Perhaps this needs to be explicitly added to authenticator-rs's Cargo.toml, and then we can resolve whatever the link error is, there.

I've opened https://github.com/mozilla/authenticator-rs/issues/128 to look into this in the lib, maybe with an explicit dependency, or maybe a switch to https://crates.io/crates/udev - donno yet.

Flags: needinfo?(jjones)
Attachment #9129977 - Attachment is obsolete: true

jcj, Do you think you own this bug? Or delegate it to someone who know vendoring rust code better than I? :)

Assignee: sledru → nobody
Flags: needinfo?(jjones)

I'm taking a look at it right now, and I think, uh, dom/webauthn/libudev-sys/ is very much not the crate in crates.io; it looks like Tim rewrote it from scratch to support dynamic loading, since Firefox has to dynamically load libudev.so.

It's deliberate that it's name-colliding with the crate of the same name, as it's basically a monkeypatch. As a -sys crate, it's just the extern "C" functions and structs, and won't change much.

But the key pieces there are the lazy_static dlopen and the library tracking.

Turns out I opened an issue on the parent lib in 2017: https://github.com/dcuddeback/libudev-sys/issues/1 .

What I think I need to do here is to take this bug and do two things:

  1. provide a PR to dcuddeback/libudev-sys with the mozilla-central changes
  2. add a README to the m-c folder explaining all this.

Then sometime in the future we can look to vendor in the original lib, if our PR gets accepted.

Let me know if you want some other outcomes.

Flags: needinfo?(jjones) → needinfo?(sledru)

uh, oof, actually the whole lib.rs has to get replaced. Even if I make dynamic loading a feature on the crate, I'd basically need to make build.rs choose between completely different implementations.

I am guessing we were hoping bindgen would magic itself dynamic ffi, but that appears to still be an open feature request.

So providing a PR back to the author isn't going to be a quick process. Maybe we can just put a README in-tree? Maybe also a comment in one of the gecko .tomls ?

for when it's relevant: https://github.com/jcjones/libudev-sys has the m-c changes in it. I'm just going to stop there for now.

So just confirming my understanding here -- the reason we need to actually dlopen libudev is so we can allow the linux gamepad system to not exist?

Assignee: nobody → a.beingessner

Wait sorry, even stronger: so that we can defer the cost of the dynamic linker looking up libudev until you visit a page that uses the JS gamepad API? To make process startup times faster? Is this a general policy, or just something that we took special care with udev for?

Chris, do you have an opinion on comment #15? thanks!

Flags: needinfo?(sledru) → needinfo?(cmartin)

(In reply to Alexis Beingessner [:Gankra] from comment #15)

Wait sorry, even stronger: so that we can defer the cost of the dynamic linker looking up libudev until you visit a page that uses the JS gamepad API? To make process startup times faster? Is this a general policy, or just something that we took special care with udev for?

Sorry to not be able to provide a more certain answer here, but I wasn't around when the design decisions behind this were being made (and afaik everyone that was around no longer works here). I've been working on this code for a little while now, so I'll give my best educated guess.

My impression is that a fair bit of effort was put into making all the Gamepad API code almost-completely dynamic. (Probably because gamepad use in Firefox is relatively rare.)

If the JS Gamepad API isn't being used by a content process, GamepadManager and GamepadPlatformService won't be allocated, the IPDL channel between them won't be created, and none of the OS-specific APIs will be initialized (RawInput/XInput on Windows, IOHIDManager on Darwin, libudev on Linux). These things are also eagerly destroyed as soon as all JS Gamepad objects are finalized. In effect, the gamepad infrastructure has almost-zero cost if you aren't actively using it.

So my best guess would be that dynamically loading udev is just a downstream consequence of the fact that all Gamepad stuff is dynamically loaded, and not anything special about udev specifically. We do the same thing on Windows with xinput1_4.dll.

Aside

I don't completely understand everything up there ^^^ about this Rust libudev-sys crate, but AFAICT the Gamepad code does the dlopen() and dlsym() calls itself here and doesn't invoke any Rust functions to do it. I don't know if this "monkey patching" thing changes that, but are you certain that the Gamepad code is using this Rust crate?

Flags: needinfo?(cmartin)

Oh ok I see, I just instinctively assumed the rust uses of libudev were for the same reasons as the cpp ones, and that's wrong.

Actually, we only use libudev-sys for the authenticator crate, which in turn is pulled in by gkrust-shared... but then seemingly never used?

Is authenticator-rs abandoned/dead code?

Or perhaps it hasn't actually been fully hooked up yet.

Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/176c0f345637
Properly explain our fork of libudev-sys. r=sylvestre DONTBUILD

https://hg.mozilla.org/mozilla-central/rev/176c0f345637

Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox85: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: