Bug 1657292 - Ping registration off main thread

[badboy]

I don't like the second commit that disables the metrics ping by setting the last sent date, awkardly.
Not sure why that is necessary, at a minimum we should have a nicer way to deal with it.

Otherwise this seems to work in our tests.
We would need to take this to a test drive in Fenix to figure out if that fixes the startup delay for them.

[mdboom]

I think this is fine, since we only send built-in pings (defined in the Rust core) at startup.

This does indicate that we have a bug: custom pings that include events should also be flushed at startup because they have the same timing issues as the events. But that's a separate bug and not a live issue right now.

[badboy]

I think this is fine, since we only send built-in pings (defined in the Rust core) at startup.

This does indicate that we have a bug: custom pings that include events should also be flushed at startup because they have the same timing issues as the events. But that's a separate bug and not a live issue right now.

We already do that.
We do check for pending events on startup and send those out in their corresponding pings. For that we need to have that ping registered (because we need to know if it should include the client ID).
For most user (and for all that I know) events always only land in the events ping, so it's no problem (we register that ping synchronously before we need it), but in theory we could have a custom ping that also has events.

[badboy]

I asked for help with profiling. Until then we shouldn't merge it.

[badboy]

I think I detected a problematic case that's currently hidden from the user. This patch would make the behavior slightly more consistent and obvious.

Consider this order of calls (Android, current code in main):

Glean.registerPings(Pings)
// ...(1)
Glean.initialize(...)

((1) is arbitrary user code in between those calls)
Custom Pings would always be registered before startup, thus overdue events in custom pings would get sent out correctly.

Now consider this:

Glean.initialize(...)
// ...(2)
Glean.registerPings(Pings)

((2) is arbitrary user code in between those calls)
If there's nothing in (2) and everything is fast the custom Pings might be added before Glean.initialize reaches on_ready_to_submit_pings. Thus overdue events are sent in that custom ping.

If (2) is slow, custom Pings might be registered after on_ready_to_submit_pings. Thus overdue events are discarded (because that ping is unknown).

Now consider that Glean.initialize and Glean.registerPings could be called from different threads even, thus there is no obvious order of calls in the code to begin with.

---

With this patch we guarantee that, if registerPings happens after initialize returns (note: full init might still be running off the main thread) it only takes effect after full initialization. No overdue events are sent for these custom pings.
We would need to update the docs and recommend registration before initialize.

The same caution applies when calling from different threads.
Fenix calls registerPings in parallel to initialize.

@mdboom @travis79 Please check if my analysis of the current complex async behavior is correct here and let me know if we should land this PR.

Note: this was initially introduced to prevent a potential startup delay for Fenix, I am still unable to reproduce that in a profile to verify this patch helps.

[badboy]

Another note: there's now a high likelyhood the tests fail (as they did on CI) because we potentially queue too many ping register tasks in tests, causing the queue to spill over.

[badboy]

I modified it slightly and changed tests.
Please give it a thorough review

[travis79]

Overall, I'm pretty happy with this. I think this takes care of the possibility of losing the ping registrations due to overflowing the pre-init queue, and cuts out another source of async weirdness. Good work!

[mdboom]

This looks good. I agree the removed test doesn't really test anything correctly anymore.

Are there C# changes also required? Since we don't have any C# users, happy to just file a bug and handle that later.

[badboy]

This looks good. I agree the removed test doesn't really test anything correctly anymore.

Are there C# changes also required? Since we don't have any C# users, happy to just file a bug and handle that later.

I file one.

[badboy]

https://bugzilla.mozilla.org/show_bug.cgi?id=1660815