In a normal Firefox Desktop run there'll be multiple content children and each one will need their own pair of child,parent endpoints. This'll mean that we'll have multiple different parent endpoints arriving here, all of which we'll need to bind.

This ought to be testable via xpcshell with multiple simultaneous content tasks, but I'm not sure I've ever tried.

bikeshed: mFOGTaskQueue

Please link to the Preferences and Defines doc, and include in it a section on the new pref

Weird that it's asking you to qualify these. They ought to just be from the statement up on line 49

I think modern C++ prefers over like

Prefer to

Prefer to

Seems like re-defining that statement to be has fixed the issue

I don't think you need to hold onto this task queue, as this will be the only time it is ever used - as it's created for this particular use. I also don't think it is possible for it to fail to create a queue when a is still able to receive IPC, so you can probably get away with .

FYI the current design creates a separate task queue for every content process. I'm not sure if that's what the intention was or not?

Out of curiosity, why the vowels-removed task queue name?

Where did this change come from?

Looks like you ran into the " is ambiguous" issue I mention below :-)

Do you aspire to also support for these other process types? I think it should be feasible to also send the endpoints over these actors as well, and uniformly use for FOG data between all child processes and the parent process (though non-content processes naturally don't have a type).

I notice you're still sending the FOG data on the content process main thread. I take it this patch isn't intending to change that?

I'd generally not bother with doing unless you really need to avoid a refcount.

AFAICT this is currently completely unused, is that correct? DO you have aspirations to do more validation based on this or something like that?

In general I prefer not to add extra namespaces like when possible. We have one for , and it causes headaches every once in a while, as if you're in a translation unit which has done a , or are within the namespace, names like are now ambiguous and require full qualification. This sometimes causes build issues in unified builds.

I think I'd probably recommend just putting it into ?

nothing on this protocol needs to be sync.

isn't necessary for toplevel actors.

We like to add comments to closing namespace indicators, so that it's more clear what's going on, as often they can be quite far away from the initial declaration.

The intention was for it to create a single task queue on first use that would be used across all processes, so that's certainly something I need to adjust. I'd meant for the created task queue to have be a reference to it.

Is there something in what I wrote that didn't do that as I expected it to? That's what I understood to be doing.

I think that's from when I had accidentally left it as a BackgroundThread, which had a character limit on the name. I was following previous patterns, which primarily seemed to remove the vowels.

Harbormaster and my local IDE were both complaining that usages of ByteBuf later in the file were not resolving correctly, and this seemed to resolve that issue

Yep that's also why I ended up adding the above

My understanding is for now we are focused on content processes, but we may choose to expand it in the future.

This would be the child process' main thread, right?

I think it would be wise of us to do so, but at this time I admittedly don't have the experience in this space to know how to do that.

I'm running into issues implementing this change due to the following errors:

0:13.37 E /Users/charlie.humphreys/git/firefox/toolkit/components/glean/FOGTransportParent.h:24:9: error: call to implicitly-deleted copy constructor of 'ThreadsafeContentParentHandle' (aka 'mozilla::dom::ThreadsafeContentParentHandle')
0:13.37 E 24 | : mThreadsafeContentParentHandle(*aThreadsafeContentParentHandle) {}
0:13.37 E | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
0:13.37 E /Users/charlie.humphreys/git/firefox/dom/ipc/ContentParent.h:1679:3: note: copy constructor of 'ThreadsafeContentParentHandle' is implicitly deleted because field 'mRefCnt' has a deleted copy constructor
0:13.37 E 1679 | NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ThreadsafeContentParentHandle);
0:13.37 E | ^
0:13.37 E /Users/charlie.humphreys/git/firefox/xpcom/base/nsISupportsImpl.h:771:3: note: expanded from macro 'NS_INLINE_DECL_THREADSAFE_REFCOUNTING'
0:13.37 E 771 | NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_DESTROY(_class, delete (this), \
0:13.37 E | ^
0:13.37 E /Users/charlie.humphreys/git/firefox/xpcom/base/nsISupportsImpl.h:749:3: note: expanded from macro 'NS_INLINE_DECL_THREADSAFE_REFCOUNTING_WITH_DESTROY'
0:13.37 E 749 | NS_INLINE_DECL_THREADSAFE_REFCOUNTING_META(_class, NS_METHOD_, _destroy, \
0:13.37 E | ^
0:13.37 E /Users/charlie.humphreys/git/firefox/xpcom/base/nsISupportsImpl.h:732:35: note: expanded from macro 'NS_INLINE_DECL_THREADSAFE_REFCOUNTING_META'
0:13.37 E 732 | ::mozilla::ThreadSafeAutoRefCnt mRefCnt; \
0:13.38 E | ^
0:13.38 E /Users/charlie.humphreys/git/firefox/xpcom/base/nsISupportsImpl.h:372:3: note: 'ThreadSafeAutoRefCnt' has been explicitly marked deleted here
0:13.38 E 372 | ThreadSafeAutoRefCnt(const ThreadSafeAutoRefCnt&) = delete;
0:13.38 E | ^
0:13.38 W In file included from /Users/charlie.humphreys/git/firefox/obj-aarch64-apple-darwin25.4.0/dom/ipc/Unified_cpp_dom_ipc0.cpp:119:
0:13.38 W In file included from /Users/charlie.humphreys/git/firefox/dom/ipc/ContentParent.cpp:142:
0:13.38 E /Users/charlie.humphreys/git/firefox/toolkit/components/glean/FOGTransportParent.h:28:33: error: field of type 'ThreadsafeContentParentHandle' (aka 'mozilla::dom::ThreadsafeContentParentHandle') has private destructor
0:13.38 E 28 | ThreadsafeContentParentHandle mThreadsafeContentParentHandle;
0:13.38 E | ^
0:13.38 E /Users/charlie.humphreys/git/firefox/dom/ipc/ContentParent.h:1710:3: note: declared private here
0:13.38 E 1710 | ~ThreadsafeContentParentHandle() { MOZ_ASSERT(!mWeakActor); }
0:13.38 E | ^
0:14.72 E 2 errors generated.

Figured it out!

is a per-process actor, so if you're storing the task queue on as a member, then it will still be per-content-process.

If you want to have a task queue which is global, you need to make it a static, rather than having it on the object. The easiest way is perhaps to add a (e.g. https://searchfox.org/firefox-main/rev/7c07e6f277dd6d5122183d6feecb7473277ab5bb/xpcom/tests/gtest/TestStaticBackgroundTaskQueue.cpp#14-17), which is a helper to declare background task queues which are process global.

This could look something like this:

// in the header
class FOGTransportParent : ... {
 ...
 static already_AddRefed<nsISerialEventTarget> GetQueue();
 ...
};
// in the .cpp
already_AddRefed<nsISerialEventTarget> FOGTransportParent::GetQueue() {
 static StaticBackgroundTaskQueue sQueue("FOGTransportTaskQueue");
 return sQueue.Get();
}
// when using it:
nsCOMPtr<nsISerialEventTarget> queue = FOGTransportParent::GetQueue();
...
queue->Dispatch(...);

Now that you've dropped the namespace, you can probably also drop the other changes in this file removing prefixes. They're not necessary due to the statement, but it's nice to keep the diff smaller if possible.

FWIW I think that IPDL will already generate a typedef for this on PFOGTransportChild.

FWIW I think that IPDL will already generate a typedef for this on PFOGTransportChild.

This is probably already defined with a typedef from .

This should still be a strong reference. My feedback about using a raw pointer was just for the signature of the constructor.

If you're not using it for now, you can probably drop the entirely unless you think you'll want to use it in the near future.

Does Glean actually preserve any information about which process sent the telemetry? My vague understanding is that there is no "process identity" as far as glean is concerned, so the buffers sent here are just blindly replayed onto the global state from the parent process. Is that right?

You're missing a newline at end of file.

I believe that's correct. In which case I agree, I think we can completely drop that handle.

If you're not using the threadsafe handle, there's no reason to capture it.

Let's not have a 0 refcount object sitting around in a local - best practice these days is to use like this.

this is dead code now

Looks like you still have an extra change here removing

Thing which I just noticed, sorry I didn't catch this in an earlier pass:

This patch currently doesn't change the logic such that we're sending the message over the actor. In fact, it will be sent over the normal actor like before.

I think this could have surprising consequences if the FOG transport is turned on, as it means that the flush may not actually have flushed all data (due to different threads being involved). Effectively, now that you're using a second actor, there's nothing guaranteeing that all FOG data messages sent over have arrived & been processed before the response from arrives.

This is complicated even more more by timing. Currently the actor isn't created at all until the first message is ready to be sent by the child, which means that even if we send over the actors, the parent process may have actors which don't have a corresponding actor to send the message over.

Looking at the way that is used, it appears it's used in only the following two ways:

1. During testing, to ensure that data is available in the parent before checking what data is stored (https://searchfox.org/firefox-main/rev/4524afcc186654ad357e48fec686249515860ac8/toolkit/components/glean/xpcom/FOG.cpp#392-424), and
2. During "idle" events (https://searchfox.org/firefox-main/rev/4524afcc186654ad357e48fec686249515860ac8/toolkit/components/glean/xpcom/FOG.cpp#431-438)

Neither of these appear to be super worried about the edge cases of trying to flush FOG data for a content process which is _extremely_ early during startup, so I would propose the following solution:

1. We don't want to wait until the content process is ready to send up to create the actor, instead we probably want to create it early during content process startup, and pass that actor down to the FOGIPC code, so we replace with this:

// FOGTransportChild.h

/**
 * Get the singleton FOGTransportChild instance for the current process.
 * Returns null if the FOG transport actor either hasn't been created yet, or is disabled.
 */
static FOGTransportChild* GetSingleton();

/**
 * Create the FOGTransportActor for the current process, binding it
 * to the parent process over the given endpoint.
 */
static void Create(Endpoint<PFOGTransportChild> aEndpoint);

2. Now the actor needs to be created manually early during content process startup.
   
   For this, we add some code to early during content process startup, but after IPC has been established. Arbitrarily, I think we could get away with doing the following, but feel free to tweak to something which feels better:
   • Flip the message around so that it's sent from parent->child
   • Create the endpoint pair in (maybe around here? https://searchfox.org/firefox-main/rev/4524afcc186654ad357e48fec686249515860ac8/dom/ipc/ContentParent.cpp#2972), and send the child endpoint to the content process with the message.
     • Stash the on as a new member, so we can access it later. This should be OK as it's threadsafe refcounted.
     • Don't bother creating the actor or sending the message if the pref is disabled.
   • In the content process, call from the receiver for .

3. In the content process, when flushing the FOG transport, check if there is a by checking (you could even do this before checking process type if you wanted), and send the message over the transport singleton if it's available.

4. In , replace the direct call with a wrapper which does something like this:

RefPtr<FlushFOGDataPromise> ContentParent::DoFlushFOGData() {
 if (mFOGTransport) {
 nsCOMPtr<nsISerialEventTarget> queue = glean::FOGTransportParent::GetQueue();
 return InvokeAsync(queue, __func__, [transport = RefPtr{mFOGTransport}] {
 return transport->SendFlushFOGData();
 });
 }
 return SendFlushFOGData();
}

Yeah, the "content process" is our name for the child process which renders web content.

Whoopsie, thank you

I ended up pairing with @chutten on this and we _think_ we've fully covered what you shared in this comment. Great callout, thank you for the in-depth description of what needed to be done.

If you don't mind double checking our work I think that would be very greatly appreciated.

Yep we aren't changing that at the moment, as best I'm aware