Closed Bug 1747107 Opened 1 year ago Closed 1 year ago

FrameContextUtils.jsm getOsPid is incorrect

Categories

(Remote Protocol :: Agent, defect, P2)

defect
Points:
2

Tracking

(firefox98 fixed)

RESOLVED FIXED
98 Branch
Tracking Status
firefox98 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [bidi-m3-mvp])

Attachments

(1 file)

As noted by :ochameau on https://phabricator.services.mozilla.com/D131897#inline-739600, there is actually no osPid property on the window object available on a regular BrowsingContext instance, meaning return browsingContext.window.osPid; always returns undefined.

This was not spotted in our tests, because we only use this method to compare the result with -1, and the only case where we can have -1 is when the WindowGlobal lives in the parent process, meaning its browsing context will be a CanonicalBrowsingContext.

But the method is nevertheless wrong. It actually sounds complicated to get the osPid information cross process. We should probably update the method to simply return check if the browsing context corresponds to a parent process window global or not.

function isParentProcess(browsingContext) {
  if (browsingContext instanceof CanonicalBrowsingContext) {
    return browsingContext.currentWindowGlobal.osPid === -1;
  }

  // If `browsingContext` is not a `CanonicalBrowsingContext`, then we are necessarily in a content process
  return false;
}

After discussing with :ochameau, he suggested that for our current needs (exclude chrome privileged content) a better check would be

from the parent process:

browsingContext.currentWindowGlobal.documentPrincipal.isContentPrincipal

from the content process:

browsingContext.window.document.nodePrincipal.isContentPrincipal

I tried the isContentPrincipal approach, but it doesn't filter out all parent process about: pages. Eg for about:robots, isContentPrincipal returns true. I'll probably move forward with my suggestion and in parallel we should clearly define which windows we want to accept and ask help from the DOM team.

For now excluding all parent process pages should be a decent approximation (and again, the current implementation should work, but the helper method is misleading, so it should still be fixed).

Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Severity: -- → S3
Points: --- → 2
Priority: -- → P2
Whiteboard: [webdriver:triage]
Whiteboard: [bidi-m3-mvp]
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/083b25b3bb4a
[remote] Replace FrameContextUtils::getOsPid with isParentProcess r=webdriver-reviewers,jgraham,whimboo
Flags: needinfo?(jdescottes)
Pushed by abutkovits@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46945c37c244
[remote] Replace FrameContextUtils::getOsPid with isParentProcess r=webdriver-reviewers,jgraham,whimboo. CLOSED TREE

Relanded with this push.

Sorry for the wrongful backout.

Flags: needinfo?(abutkovits)
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
You need to log in before you can comment on or make changes to this bug.