Closed Bug 1271495 Opened 4 years ago Closed 4 years ago

Replace uses of ScopedPK11Context with UniquePK11Context

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

ScopedPK11Context is based on Scoped.h, which is deprecated in favour of the
standardised UniquePtr.

By making this change, we move one step closer to removing Scoped.h.
ScopedPK11Context is based on Scoped.h, which is deprecated in favour of the
standardised UniquePtr.

Review commit: https://reviewboard.mozilla.org/r/52111/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52111/
Attachment #8751617 - Flags: review?(mcmanus)
Attachment #8751617 - Flags: review?(dkeeler)
https://reviewboard.mozilla.org/r/52111/#review49057

keeler: Please review the PSM and (the single line) dom/crypto/ changes.
mcmanus: Please review the BackgroundFileSaver changes. Feel free to redirect - hg blame seemed to suggest you would be most familiar with the code.

Thanks!
Comment on attachment 8751617 [details]
MozReview Request: Bug 1271495 - Replace uses of ScopedPK11Context with UniquePK11Context. r=keeler,mcmanus

https://reviewboard.mozilla.org/r/52111/#review49137
Attachment #8751617 - Flags: review?(mcmanus) → review+
Comment on attachment 8751617 [details]
MozReview Request: Bug 1271495 - Replace uses of ScopedPK11Context with UniquePK11Context. r=keeler,mcmanus

https://reviewboard.mozilla.org/r/52111/#review49201

LGTM.

::: security/manager/ssl/ScopedNSSTypes.h:141
(Diff revision 1)
>   *   NS_ENSURE_SUCCESS(rv, rv);
>   *   rv = MapSECStatus(SomeNSSFunction(..., digest.get(), ...));
>   *
>   * Less typical usage, for digesting while doing streaming I/O and similar:
>   *
>   *   Digest digest;

Looking at this, Digest is a really strange API (maybe I'm just not understanding it). I think we should clean it up a bit in another bug (is it ever actually used for streaming as described here?).

::: security/manager/ssl/ScopedNSSTypes.h:182
(Diff revision 1)
>      NS_ENSURE_SUCCESS(rv, rv);
>      uint32_t len;
> -    rv = MapSECStatus(PK11_DigestFinal(context, mItem.data, &len, mItem.len));
> +    rv = MapSECStatus(PK11_DigestFinal(context.get(), mItem.data, &len,
> +                                       mItem.len));
>      NS_ENSURE_SUCCESS(rv, rv);
>      context = nullptr;

Yeah, Digest is a really weird API. I think this is a strange way to structure things.
Attachment #8751617 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/52111/#review49201

> Looking at this, Digest is a really strange API (maybe I'm just not understanding it). I think we should clean it up a bit in another bug (is it ever actually used for streaming as described here?).

I filed Bug 1272794.
Comment on attachment 8751617 [details]
MozReview Request: Bug 1271495 - Replace uses of ScopedPK11Context with UniquePK11Context. r=keeler,mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52111/diff/1-2/
Attachment #8751617 - Attachment description: MozReview Request: Bug 1271495 - Replace uses of ScopedPK11Context with UniquePK11Context. → MozReview Request: Bug 1271495 - Replace uses of ScopedPK11Context with UniquePK11Context. r=keeler,mcmanus
https://hg.mozilla.org/mozilla-central/rev/c697aa342211
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.