FrameContextUtils.jsm getOsPid is incorrect
Categories
(Remote Protocol :: Agent, defect, P2)
Tracking
(firefox98 fixed)
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;
}
Assignee | ||
Comment 1•1 year ago
|
||
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
Assignee | ||
Comment 2•1 year ago
|
||
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 | ||
Comment 3•1 year ago
|
||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/083b25b3bb4a [remote] Replace FrameContextUtils::getOsPid with isParentProcess r=webdriver-reviewers,jgraham,whimboo
Comment 5•1 year ago
|
||
Backed out for causing assertion failures at nsHttpChannel.cpp.
Backout link: https://hg.mozilla.org/integration/autoland/rev/f37aad64a051f35286db803a04096cec6e7a3c12
Push where failures started: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=974475ee83ec10e42668e523bfa34ece463c4468&selectedTaskRun=PuRjNR05RZW8g_6ZYewmZg.0
Failure log: https://treeherder.mozilla.org/logviewer?job_id=366336574&repo=autoland
Assignee | ||
Comment 6•1 year ago
|
||
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
Assignee | ||
Comment 7•1 year ago
|
||
Yes, already orange with the same error: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=success%2Cpending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=remote&revision=0b12f1bb498a5f024341697b2163f725815b12d5&selectedTaskRun=SvVxrfVfQ_ew--iVbQdJZw.0
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
Comment 9•1 year ago
|
||
Relanded with this push.
Sorry for the wrongful backout.
Comment 10•1 year ago
|
||
bugherder |
Description
•