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

Comment 10

[Vincent Hilla [:vhilla]]

Attachment: Bug 2045635 - Validate more fields for nsDocShellLoadState.

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

Comment 11

[Phabricator Automation]

firefox-esr153 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: (enhancement)
• Risk associated with taking this patch: low
• Explanation of risk level: This adds some checks around loading that could cause crashes. But the change has been in Nightly for a month.
• String changes made/needed?: no
• Is Android affected?: yes

Comment 12

[Vincent Hilla [:vhilla]]

Attachment: Bug 2045635 - Validate more fields for nsDocShellLoadState.

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

Comment 13

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined/Reason for urgency: Add more principal validation in 140.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: (enhancement)
• Risk associated with taking this patch: low
• Explanation of risk level: This adds some checks around loading that could cause crashes. But the change has been in Nightly for a month.
• String changes made/needed?: No
• Is Android affected?: yes

Comment 14

[Dianna Smith [:diannaS]]

:vhilla do we have to uplift the regression(bug 2048851) to esr153 as well?

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/3f2781522efd

Comment 16

[Vincent Hilla [:vhilla]]

No, but thanks for checking. Bug 2048851 wasn't correct and I fixed that in bug 2054487, which was uplifted to esr153.

https://searchfox.org/firefox-esr153/rev/1bc46ce00a05d72817996890aac62c349814d168/docshell/base/nsDocShell.cpp#4308-4311

Comment 17

[Dianna Smith [:diannaS]]

This is being backed out of esr140 for causing reftest failures.

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/0dcceefccce6

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/a3d48abae796

Comment 20

[Vincent Hilla [:vhilla]]

(In reply to Dianna Smith [:diannaS] from comment #17)

This is being backed out of esr140 for causing reftest failures.

That makes sense for this test. A chrome:// reftest is loaded with system principal in a web remote type and embeds an iframe. So we get a system principal to inherit. I looked around and in 153, bug 2011307 changed it so that chrome:// documents always load in the parent.

I'll see if we can slightly tweak the check.

Comment 21

[Dianna Smith [:diannaS]]

:vhilla any update?

Comment 22

[Vincent Hilla [:vhilla]]

Sorry, I'm unsure what's the process when an uplift got backed out. The patch got r+, do you set approval-mozilla-esr140+ or should I land?

I tried pushing to try but lando says hg error in cmd so I was only able to test locally.

Comment 23

[Dianna Smith [:diannaS]]

if you updated the revision i can reland it.
the process is you just let me know the fix is corrected, you will see a new 140+ from me.

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/20b9273c1c78

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/55e8ed4b9cc5

Comment 26

[Dianna Smith [:diannaS]]

Backed out of esr140
https://treeherder.mozilla.org/jobs?repo=mozilla-esr140&duplicate_jobs=visible&group_state=expanded&resultStatus=unscheduled%2Cpending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2C18.04%2Casan%2Copt%2Cmochitests%2Cwith%2Csoftware%2Cwebrender%2Cenabled%2Ctest-linux1804-64-asan-qr%2Fopt-mochitest-browser-chrome-swr%2Cbc15&fromchange=702043a5ac0890fe873a7a9df9c3d62645003f30

Comment 27

[Vincent Hilla [:vhilla]]

It's again due to bug 2011307, which removed the failing test. The subtest remote -> chrome via clickLink times out.

I had to git fetch hg::https://hg.mozilla.org/releases/mozilla-esr140 to be able to resolve the hg error in cmd lando errors.

Push to try with a fix looks good: https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=78041

Comment 28

[Vincent Hilla [:vhilla]]

:diannaS, can you land again? I updated the patch to address the test failure.

Comment 29

[Dianna Smith [:diannaS]]

We already built esr140 since we are in RC week. It will get picked up to land next week for the next cycle.

Comment 30

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/4f080e398a7f