2045635 - Validate more fields around docshell load state

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[Vincent Hilla [:vhilla]]

To follow-up on bug 2027207 validating principals, we can also add checks to load state.

Comment 1

[Vincent Hilla [:vhilla]]

Attachment: Bug 2045635 - Validate more fields for nsDocShellLoadState. r=nika

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b03ef05776a6
https://hg.mozilla.org/integration/autoland/rev/afee3fe942f5
Validate more fields for nsDocShellLoadState. r=nika

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/08c4c75efd4b
https://hg.mozilla.org/integration/autoland/rev/65c5f5a151c2
Revert "Bug 2045635 - Validate more fields for nsDocShellLoadState. r=nika" for causing failures at browser_open_docgroup.js.

Comment 4

[Atila Butkovits]

Backed out for causing failures at browser_open_docgroup.js.

Backout link: https://hg.mozilla.org/integration/autoland/rev/65c5f5a151c2

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=linux%2C24.04%2Copt%2Cmochitests%2Cwith%2Csoftware%2Cwebrender%2Cenabled%2Ctest-linux2404-64%2Fopt-mochitest-browser-chrome-swr%2Cbc4&revision=afee3fe942f56482b3259978cfe4e24bef40171b&selectedTaskRun=QFukJftpRO621oooWa3qXQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=572199629&repo=autoland&task=QFukJftpRO621oooWa3qXQ.0&lineNumber=17496

Comment 5

[Vincent Hilla [:vhilla]]

Looking at my try history, the tests passed previously which is confusing. What's failing is the check for mPartitionedPrincipalToInherit. I remember I had the same happen locally once but wasn't able to reproduce it consistently.

 0:03.55 GECKO(39037) [MozDbg] [./../../../docshell/base/nsDocShellLoadState.cpp:193] mPartitionedPrincipalToInherit = nsIPrincipal { http://example.com^partitionKey=%28http%2Cexample.com%29 } @ 0x7a379b707dc0
 0:03.56 GECKO(39037) [MozDbg] [./../../../docshell/base/nsDocShellLoadState.cpp:194] GetEffectiveTriggeringRemoteType() = "webIsolated=https://example.com"
 0:03.56 GECKO(39037) [MozDbg] [./../../../docshell/base/nsDocShellLoadState.cpp:195] mPrincipalToInherit = nsIPrincipal { moz-nullprincipal:{041f6b76-256e-4969-b90a-576a1c9307f7}?http://example.com } @ 0x7a379c5f6300

Note PPTI is http while remote type is https. That seems like a bug, I'll try to figure out how the principals get out of sync. It gets fixed when changing example.com to https. Reminds me of bug 2003449.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/aa0af8853008
https://hg.mozilla.org/integration/autoland/rev/4d437225e2ab
Validate more fields for nsDocShellLoadState. r=nika

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4d437225e2ab

Comment 8

[Vincent Hilla [:vhilla]]

Attachment: Bug 2045635 - Validate more fields for nsDocShellLoadState.

Original Revision: https://phabricator.services.mozilla.com/D305101

Comment 9

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Add more principal validation in 153.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: medium
• Explanation of risk level: This adds some checks around loading and more edge cases like bug 2048851 might show up. The change has been in Nightly for a month, edge cases would likely be low severity.
• String changes made/needed?: No
• Is Android affected?: yes