Open Bug 1646548 Opened 4 years ago Updated 1 year ago

Add comment explaining GetInProcessParentDocument's Fission behavior in WorkerPrivate::GetLoadInfo

Categories

(Core :: DOM: Workers, task, P3)

task

Tracking

()

ASSIGNED
Fission Milestone Future

People

(Reporter: kmag, Assigned: asuth)

References

(Blocks 1 open bug)

Details

It's meant to walk the parent document chain until it finds a document without a sandboxed origin, but it stops as soon as it encounters one which is OOP.

Severity: -- → S2
Priority: -- → P2
Summary: Fix GetInProcessParentDocument usage in GetInProcessParentDocument → Fix GetInProcessParentDocument usage in WorkerPrivate::GetLoadInfo

Not sure the existing behavior is possible with fission. asuth, can you please look into what is it trying to do here and if it will need to be fixed for Fission?

Fission Milestone: --- → M6b
Flags: needinfo?(bugmail)

The current use appears to be fine. I'm going to provide a patch to add a comment that clarifies that the logic is sound under Fission and explains why. If there is an alternate method signature that should be used that's intended to be "I've considered the fission implications", please let me know and I'll switch the code to use that too, but I didn't see one (for Document, I believe there are some for other types).

Restating:

  • Per Process Per Domain Worker Limit:
    • Workers has a per-process mechanism that limits the maximum number of actively running workers that can be spawned by a domain (not origin! eTLD+1). Currently the limit is 512 and pref-overrideable.
    • This is different than the historical (per-process) per-domain limit of 10 active workers which was removed in bug 1280241. (Any number of Workers could be queued up, however, which created potential content-space deadlocks if they were waiting for a Worker to do something.)
    • However, bug 1286895 which introduced the 512 limit it was implemented by just re-introducing that mechanism which was removed in bug 1280241 with its much higher limit.
    • The intent of this limit was to avoid fork bombs (OS-level denial of service attack) and avoid filling the address space with stacks (a mechanism to make life easier for in-process attacks)
  • Sandbox-piercing:
    • The logic that calls GetInProcessPazrentDocument in WorkerPrivate::GetLoadInfo is attempting to locate the domain that created the sandboxed iframe and therefore is the responsible party that the worker should be billed against.
    • Per question raised at https://matrix.to/#/!DAzdivXKbExPfeQEjo:mozilla.org/$XMqPM8w2VQJC_Ry3SJsYmaCzNDlZVKjMBnWTiiRT_Yw?via=mozilla.org&via=matrix.org&via=t2bot.io and subsequent discussion, although the null principals used by sandboxed iframes could in theory end up in any number of places, it's likely that the intentional policy will be for sandboxed iframes to remain in the same process as the creator of the sandboxed iframe.
    • This means that the logic should continue to work for its current purposes.
      • Given that our goal is to bound the number of worker threads that can be created in a given process and we should be able to properly attribute sandboxed iframes, this means our per-process accounting will be accurate. And even more accurate at a profile-wide level under fission than it was without fission!
    • The question is whether there is a better alternate mechanism to get this information.
      • I don't think there is at this time. The logic that creates the null principal inherits the origin attributes of the parent, but there's no linkage between that null principal and its parent and the ClientSource and I don't think there's any other comparable mechanism that would track this relationship. (It might make sense for ClientSource to expose some of this hierarchy for about:performance's benefit though? Or maybe its data gathering should already be aware of the hierarchy?)
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Flags: needinfo?(bugmail)

The patch with the comment is not needed for M6b.

Fission Milestone: M6b → M6c

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #2)

The current use appears to be fine. I'm going to provide a patch to add a comment that clarifies that the logic is sound under Fission and explains why.

In that case, this bug doesn't need to block shipping Fission MVP.

Fission Milestone: M6c → Future
QA Whiteboard: qa-not-actionable
Severity: S2 → N/A
Type: defect → task
Priority: P2 → P3
Summary: Fix GetInProcessParentDocument usage in WorkerPrivate::GetLoadInfo → Add comment explaining GetInProcessParentDocument's Fission behavior in WorkerPrivate::GetLoadInfo
You need to log in before you can comment on or make changes to this bug.