Closed Bug 1747107 Opened 2 years ago Closed 2 years ago

FrameContextUtils.jsm getOsPid is incorrect

Categories

(Remote Protocol :: Agent, defect, P2)

Product:
Remote Protocol
Component:
Agent
Type:
defect
Points:
2

Tracking

(firefox98 fixed)

Status:
RESOLVED FIXED
Milestone:
98 Branch
Tracking Flags:
Tracking Status
firefox98 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [bidi-m3-mvp])

Attachments

(1 file)

Bug 1747107 - [remote] Replace FrameContextUtils::getOsPid with isParentProcess
48 bytes, text/x-phabricator-request
Details | Review

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

Let's wait for the retriggers to finish, but I can see similar failures in the live log as early as https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=remote&revision=0b12f1bb498a5f024341697b2163f725815b12d5 , which means it would rather be Bug 1751367 or Bug 1151941

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)

https://hg.mozilla.org/mozilla-central/rev/46945c37c244

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox98: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
You need to log in before you can comment on or make changes to this bug.