Audit the callers of the two-argument OriginAttributes constructor

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Bobby Holley (parental leave - send mail for anything urgent), Assigned: allstars)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Looking through the code, I see a lot of callers that follow the letter of OriginAttributes but not the spirit:

https://dxr.mozilla.org/mozilla-central/rev/6256ec9113c115141aab089c45ee69438884b680/dom/quota/QuotaManager.cpp#5291

Stuff like that isn't going to work when we add signedPkg etc. We should audit and fix these consumers, and possibly remove that constructor.
(Assignee)

Updated

2 years ago
Assignee: nobody → allstars.chh
(Assignee)

Comment 1

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b08d94df2937
(Assignee)

Comment 2

2 years ago
Created attachment 8699420 [details] [diff] [review]
Patch.
(Assignee)

Comment 3

2 years ago
Comment on attachment 8699420 [details] [diff] [review]
Patch.

Review of attachment 8699420 [details] [diff] [review]:
-----------------------------------------------------------------

Hi, Janv
Could you help to review QuotaManager part for me?
I only updated OriginProps and some code in OriginParser.

Thanks
Attachment #8699420 - Flags: review?(jvarga)

Comment 4

2 years ago
Comment on attachment 8699420 [details] [diff] [review]
Patch.

Review of attachment 8699420 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if you address my comments

::: dom/quota/ActorsParent.cpp
@@ +5988,5 @@
>      nsCString spec;
> +    PrincipalOriginAttributes attrs;
> +    bool result = OriginParser::ParseOrigin(NS_ConvertUTF16toUTF8(leafName),
> +                                            spec, &attrs);
> +    NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);

please don't use NS_ENSURE_TRUE(), see my comment below

@@ +6170,5 @@
>          }
>  
> +        nsCOMPtr<nsIPrincipal> principal =
> +          BasePrincipal::CreateCodebasePrincipal(uri, originProps.mAttrs);
> +        NS_ENSURE_TRUE(principal, NS_ERROR_FAILURE);

please no more NS_ENSURE_TRUE(), use this instead:

if (NS_WARN_IF(!principal)) {
  return NS_ERROR_FAILURE;
}
Attachment #8699420 - Flags: review?(jvarga) → review+
(Assignee)

Comment 5

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba6ecf578741
(Assignee)

Comment 6

2 years ago
Created attachment 8699808 [details] [diff] [review]
Patch. v2

addressed janv's comment.
Attachment #8699420 - Attachment is obsolete: true
Attachment #8699808 - Flags: review+
(Assignee)

Comment 7

2 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/63254aa6dfabeced552c9401cf9fb3cfa9b7add4
Bug 1209349 - Audit the callers of the two-argument OriginAttributes. r=janv

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/63254aa6dfab
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.