Closed Bug 1635399 Opened 5 years ago Closed 4 years ago

Make mozilla::ipc::PrincipalInfoToPrincipal always return Result<nsCOMPtr<nsIPrincipal>, nsresult>

Categories

(Core :: General, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox78 --- fixed

People

(Reporter: ssengupta, Assigned: ssengupta)

References

Details

Attachments

(1 file, 17 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review

mozilla::ipc::PrincipalInfoToPrincipal should return nsresult in all situations, where it cannot return nsCOMPtr<nsIPrincipal>. Therefore MOZ_CRASH is not called in any error situation in this method. The caller can decide how to best go forward.

Assignee: nobody → ssengupta
Status: NEW → ASSIGNED
Severity: -- → S3
Priority: -- → P3
Priority: P3 → P2
Attachment #9145772 - Attachment is obsolete: true
Attachment #9145773 - Attachment is obsolete: true
Attachment #9145774 - Attachment is obsolete: true
Attachment #9145775 - Attachment is obsolete: true
Attachment #9145776 - Attachment is obsolete: true
Attachment #9145777 - Attachment is obsolete: true
Attachment #9145778 - Attachment is obsolete: true
Attachment #9145779 - Attachment is obsolete: true
Attachment #9145780 - Attachment is obsolete: true
Attachment #9145781 - Attachment is obsolete: true
Attachment #9145782 - Attachment is obsolete: true
Attachment #9145783 - Attachment is obsolete: true
Attachment #9145784 - Attachment is obsolete: true
Attachment #9145785 - Attachment is obsolete: true
Attachment #9145786 - Attachment is obsolete: true
Attachment #9145787 - Attachment is obsolete: true
Attachment #9145788 - Attachment is obsolete: true
Pushed by apavel@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/af27682c5e7f Function PrincipalInfoToPrincipal now returns Result<nsCOMPtr<nsIPrincipal>, nsresult> r=ckerschb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78

Comment on attachment 9145813 [details]
Bug 1635399 - Function PrincipalInfoToPrincipal now returns Result<nsCOMPtr<nsIPrincipal>, nsresult> r=ckerschb

Beta/Release Uplift Approval Request

  • User impact if declined: Exposure to frequent crashes related to missing origin when returning nsIPrincipal pointer from PrincipalInfo data structure. This method is used throughout various components of DOM, Netwerk, and more.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This change is not very risky, because it returns mozilla::Result. Therefore there is no nullptr return to be handled. However, the PrincipalInfoToPrincipal method is used in a lot of critical pieces of code, which makes changes to this method inherently risky to an extent.
  • String changes made/needed:
Attachment #9145813 - Flags: approval-mozilla-beta?

Subhamoy, are you requesting uplift because we could have more crashes in 77 than in 76 without this patch?

Flags: needinfo?(ssengupta)

Comment on attachment 9145813 [details]
Bug 1635399 - Function PrincipalInfoToPrincipal now returns Result<nsCOMPtr<nsIPrincipal>, nsresult> r=ckerschb

Too big of a refactoring to uplift late in the beta cycle for the potential benefit, let's let it ride the train and see in beta 78 if the volume of crashes gfixed by these patches goes down, thanks for the context given on slack!

Attachment #9145813 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Flags: needinfo?(ssengupta)
Regressions: 1639170
Blocks: 1639170
No longer regressions: 1639170
Blocks: 1564107
Regressions: 1723954
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: