2053922 - Inaccurate documentation for (worker) associatedBrowsingContextID in nsILoadinfo.idl (misleading claims about process and IPC)

Status: RESOLVED FIXED

(Core :: Networking, task, P3)

Description

[Rob Wu [:robwu]]

nsILoadInfo.idl specifies associatedBrowsingContextID which was introduced in bug 1819570 with the following comment:

  /**
   * The BrowsingContext which the worker is associated.
   *
   * When used with workers:
   * Note that this could be 0 if the load is not triggered in a WorkerScope.
   * This value is only set and used in the parent process for some sitautions
   * the channel is created in the parent process for Workers. Such as fetch().
   * In content process, it is always 0.
   * This value would not be propagated through IPC.
   *

Even if these claims were true at introduction in bug 1819570, they are no longer accurate today:

• "In content process, it is always 0." is false. Patch P2 of bug 2013043 changed this. And a patch is being proposed in bug 2048884 that sets the value as well.
• "This value would not be propagated through IPC." is false - Patch P1 of bug 1819570 changed this (diff)

The documentation should either become accurate, or the stated invariants should be enforced (probably the former is more realistic).

Comment 1

[Eden Chuang[:edenchuang]]

(In reply to Rob Wu [:robwu] from comment #0)

nsILoadInfo.idl specifies associatedBrowsingContextID which was introduced in bug 1819570 with the following comment:

  /**
   * The BrowsingContext which the worker is associated.
   *
   * When used with workers:
   * Note that this could be 0 if the load is not triggered in a WorkerScope.
   * This value is only set and used in the parent process for some sitautions
   * the channel is created in the parent process for Workers. Such as fetch().
   * In content process, it is always 0.
   * This value would not be propagated through IPC.
   *

Even if these claims were true at introduction in bug 1819570, they are no longer accurate today:

• "In content process, it is always 0." is false. Patch P2 of bug 2013043 changed this. And a patch is being proposed in bug 2048884 that sets the value as well.
• "This value would not be propagated through IPC." is false - Patch P1 of bug 1819570 changed this (diff)

The documentation should either become accurate, or the stated invariants should be enforced (probably the former is more realistic).

Yes, the comment probably needs to be updated.

The line that "This value would not be propagated through IPC" means it would not be propagated by LoadInfoArgs when I created this attribute. Because fetch() in Workers(using PFetch) no longer creates a channel in the content process. Therefore, I commented on the nsILoadInfo.idl to highlight that the associatedBrowsingContextID is not propagated through LoadInfoArgs, but PFetch then sets it in the parent process channel's LoadInfo. Considering the fact that the channel is not created in the content process for Workers, this is still true with "When used for Worker."

In bug 2013043, reporting API wants to propagate the associatedBrowsingContextID for the main thread fetch, which the channel is still created in the content process. So associatedBrowsingContextID is propagated through LoadInfoArgs in the case.

So the comment could be simplified to

  /**
   * The BrowsingContextID with which the fetch() is associated.
   *
   * Fetch is used to send reports to endpoints. The associated bc ID is
   * used so that a web developer can track the sending of reports in devtools,
   * by ultimately setting the browsing context ID on a LoadInfo object.
   * 
   * This will be 0 when the fetch() is from ShardWorker and ServiceWorker.
   */

We can remove the "When used with workers" part. No channel is created during the content process for fetch() in the Worker, so there is no nsILoadInfo in the content process either. When a developer sees the channel for Workers, it must be in the parent process, and the associatedBrowsingContextID still represents the fetch() associated one. We don't need to highlight it for the Worker case, since it is not used only for Workers.

Comment 2

[Rob Wu [:robwu]]

Documentation fixed by patch to bug 2048884.