[AC-152] Fixes moz ads telemetry UniFFI callback leak

[thesuzerain]

This provides the shutdown function for addressing the crash on early ads-client -> HNT builds. When quit, we get:

console.error: "UniFFI Callback interface error during xpcom-shutdown: Error: UniFFI interface MozAdsTelemetry has 1 registered callbacks at xpcom-shutdown.

This requires a surface to deload several hanging parts of ads-client, specifically:

• any open sqlite connections (matching some other patterns elsewhere in application-services)
• any references hanging uniffi callbacks
  We provide such a function here, for use in upcoming future m-c PR to use on shutdown.

Notes

• I'm not sure if there's a good way to do a test here that would also catch any future callbacks added.
• We can reduce LoC here by putting a mutex inside the MozAdsTelemetryWrapper, or alternatively creating a new trait like TryTelemetry below that optionally record, that gives a new method to Option<dyn Telemetry>, and swapping all calls to use TryTelemetry instead. I prefer the explicit usage as shown in the PR, with if let Some(xxx) but not tied to it if we prefer one of these.

pub trait TryTelemetry {
    fn try_record(&self, event: &dyn Any);
}

impl<T: Telemetry> TryTelemetry for Option<T> {
   ...
}

Pull Request checklist

• Breaking changes: This PR follows our breaking change policy
  • This PR follows the breaking change policy:
    • This PR has no breaking API changes, or
    • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
• Quality: This PR builds and tests run cleanly
  • Note:
    • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
    • If this pull request includes a breaking change, consider cutting a new release after merging.
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
  • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
• Dependencies: This PR follows our dependency management guidelines
  • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

[Almaju]

I'm on board with dropping the caller-provided telemetry callback, but instead of Option<T>, could we default to a NoopTelemetry implementation?

My concern with Option<T> is that it pushes the "is telemetry configured?" check out to every call site and if we end up needing telemetry in more places, that cost multiplies. It's a bit like the log!() macro: having to write if tracing::is_configured() { log!() } everywhere would be pretty annoying, and I'd argue that's the telemetry layer's job to handle, not the caller's.

There's also a longer-term angle: we may want internal telemetry eventually, either by using Glean directly inside a-s components or if we migrate to a different system. A no-op default gives us a seam for that without touching call sites.

If we do want to keep the Option internally, one option is a thin telemetry wrapper that holds the Option<Box<dyn Telemetry>> (or a reference to the trait object) and exposes the same infallible API, so the branching lives in one place.

[Almaju]

If we push the concept of the telemetry wrapper (just as a brain teaser not something I think we need) that could be something like:

struct Telemetry {
    layers: Mutex<HashMap<String, Arc<dyn TelemetryLayer>>>,
}

impl Telemetry {
    fn new() -> Self {
        Self { layers: Mutex::new(HashMap::new()) }
    }

    // builder-style so it can be chained in `new`
    fn add_layer(self, key: &str, layer: Arc<dyn TelemetryLayer>) -> Self {
        self.layers.lock().unwrap().insert(key.to_string(), layer);
        self
    }

    fn remove_layer(&self, key: &str) {
        self.layers.lock().unwrap().remove(key);
    }

    fn record(&self, event: &Event) {
        for layer in self.layers.lock().unwrap().values() {
            layer.record(event);
        }
    }
}

struct SomeClient {
    telemetry: Telemetry,
}

impl SomeClient {
    fn new(glean_telemetry: Arc<dyn TelemetryLayer>) -> Self {
        let telemetry = Telemetry::new()
            .add_layer("some", Arc::new(SomeInternalTelemetry))
            .add_layer("log", Arc::new(LoggerTelemetry))
            .add_layer("glean", glean_telemetry);
        Self { telemetry }
    }

    fn shutdown(&self) {
        self.telemetry.remove_layer("glean");
    }
}

[thesuzerain]

Sure, I mention something similar in the text of the post- mostly my lean towards doing it this way was avoiding mutexes, but that's not too important. The layering makes sense. I like the hashmap solution here (though probably not a string) but definitely not needed until we have such internal telemetry.

[Almaju]

Oh yeah good catch, I think we canremove the Mutex and just use mut there!
So for now something like:

fn shutdown(&mut self) {
  self.telemetry = NoopTelemetry;
}

[thesuzerain]

Well I do think we end up with a bit of a refactor here either way. For instance: MARSTransport<T: Telemetry> (and all the other calls) have a generic T (referring to the telemetry type) rather then a pointer so it can't be substituted for another T. So that strategy would require changing all our generics for pointers. Alternatively, if we put the equivalent function inside MozAdsTelemetryWrapper, we would need to add a mutex.

So we need to do one of:

1. refactor the uses of <T: Telemetry> to use Box<dyn Telemetry> instead, then we can do in MARSTransport (etc):

fn shutdown(&mut self) {
  self.telemetry = MozAdsTelemetryWrapper::noop();
}

2. put a mutex inside of MozAdsTelemetryWrapper. Then, in MozAdsTelemetryWrapper we can do something like:

fn shutdown(&mut self) {
  let mut inner = inner.lock();
  self.inner = MozAdsTelemetryWrapper::noop();
}

4. Implement TryTelemetry and try_record() for Option<T: Telemetry> instead, to keep the Option as it is in the PR now, but we avoid needing to do if let Some(xxx) and all the extra calls as it can be handled in try_record. But this is a bit of a weird pattern and one I don't prefer.

I'm totally happy to use mutexes here (2), as we are already using them elsewhere, and the work here seems 90% synchronous anyway. I was just hoping to avoid it if it all other things were equal.

[thesuzerain]

Actually, with the understanding that we may be doing multiple layers of telemetry in the future, that adds to the reasoning to use mutexes/strategy 2.

[Almaju]

Oh yeah you are totally right! Well, Mutex, Box dyn or wrapper struct, I feel like all of them sounds okay as long as the implementation details of telemetry stuff don't leak into the rest of the code.

[Almaju]

nit: this feels similar to https://doc.rust-lang.org/std/ops/trait.Drop.html
Maybe we could have some Shutdown trait, that could make things clearer?

[thesuzerain]

I was matching an existing a-s pattern for shutdowns of sqlite here (w.r.t recursive shutdowns) but if this worked on drop and no phabricator change is required (or at least, as a backup), I agree that would certainly be interesting and could be good to do. I will try based on convo below.

I've no objection to turning this into a trait so happy to do so.

[Almaju]

Drop might not be suitableas we can't control the GC :'(

[Almaju]

Or at least we might need an exposed FFI drop / shutdown function

[Almaju]

Just curious but what would happen if we drop the client in HNT? Like if we do something like delete AdsClient.instance in JS code. Would that call Drop trait?

A risk that I anticipate is we can "shutdown" the client and then we get panic when we try to call it back whereas if we could just drop it there would be less risk of making a mistake.

In theory this should not happen because surface code would be careful and shutdown only on exit but better safe than sorry I guess?

[thesuzerain]

Revised:

• Extended the telemetry trait to require a shutdown function
• Cut out a lot of LoC about the if let Some now that it uses a mutex
• Some doc changes

Some thoughts:

• As discussed it may be possible there is a race condition with GC if we base this on Drop- it may still be possible, but I think it should be separate logic. At best we would need to explicitly deallocate/drop in JS side, at which point we may as well use a shutdown function.
• I'm not sure if we get a lot out of having a new Shutdown trait. There is shared behaviour between different components but I'm not seeing any need to reference them by trait yet, especially if we are not hooking it up to Drop.
• The test I wrote will only catch if the telemetry one is left hanging. If we have more callbacks that are potentially hanging, we would want to test to catch this issue, but I'm not sure of an obvious way to do so, so I'll note this as a possible HNT test on the javascript side we run as a part of AC-162? I don't see a great way to catch this as a test on the rust side.

A risk that I anticipate is we can "shutdown" the client and then we get panic when we try to call it back whereas if we could just drop it there would be less risk of making a mistake.

In theory this should not happen because surface code would be careful and shutdown only on exit but better safe than sorry I guess?

I agree. One thing we could consider doing is actually ensuring shutdown is safe to call without issues? Currently we make no promises about this, but it should be safe as is- any requests simply skips all telemetry and http caching. If we are confident this will always be the case (that we can shutdown what we need to and have A-C still work), I can add it to the function promise.

[Almaju]

So I would say regarding the "panic risk":

1. for telemetry, we might move to glean-sym at some point and internalize the telemetry so I think it's fine for now
2. for ContextIdProvider @copyrighthero - same this is glue code and we should internalize context-id at some point
3. for db, I think we can restart the connection if there is a need so the way I see it is rather than shutdown it's would be some sort of sleep / inactive mode and we can always "wake up" if we need so no risk to panic (but we might want this wake up logic in the db code later / separate ticket?)
   Thinking out loud a little bit so let me know if you see it differently!

[mhammond]

this is dropping the old inner, which is a uniffi call and quite possibly a call into JS - another option would be to use, say, an Option - eg, Arc<RwLock<Option<Arc<dyn MozAdsTelemetry>>>> (which is still a mouthful), shutdown is something like let _dropped = self.inner.write().take(); (which should drop the lock before inner is actually dropped), and record then needs something like:

let Some(inner) = self.inner.read().clone() else {
    return;
};

The risk with holding the lock while running foreign code is that somehow that code makes a call which tries to access the same lock, which will deadlock. That's probably unlikely though? There is also an argument that Option better reflects the semantics rather than swapping the noop one out though, and it looks like you might be able to kill NoopMozAdsTelemetry entirely?

just some driveby food for thought

[thesuzerain]

This makes good sense to me. Implemented + tested + still works OK.

[Almaju]

Suggested change

	- Add `AdsClient::shutdown_client()`, which closes the database connection early so it happens before Firefox Desktop's late-write shutdown barrier rather than during GC. In addition, this drops all held UniFFI callbacks (held in the MozAdsTelemetryWrapper) to avoid crash on a Firefox Desktop quit.
	- Add `AdsClient::shutdownt()`, which closes the database connection early so it happens before Firefox Desktop's late-write shutdown barrier rather than during GC. In addition, this drops all held UniFFI callbacks (held in the MozAdsTelemetryWrapper) to avoid crash on a Firefox Desktop quit.

small typo I think?

[Almaju]

oh no okay this is shutdown for FFI and shutdown_client for AdsClient

[Almaju]

If that first call fail, the second one will never run.

let db_result = self.client.shutdown_db();
self.telemetry.shutdown();
db_result

[Almaju]

Also I am not a big fan of comments that describe exactly the code 🤔 //Shutdown DB
And maybe it should be self.telemetry.drop() as you mention "Drop telemetry" in the comment but actually do telemetry.shutdown()

[thesuzerain]

Good call, I've reordered them for this reason

[Almaju]

What happens in term of idempotency if you were to call that method twice? Would the second one fail?

[thesuzerain]

As it stands, its fine if it runs twice. But I think until we are sure that we want things to work after the call (eg: with regards to future things we need to offload), we should not make that promise yet