1366584 - Add initial [must_use] properties to PSM IDL files

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[:Cykesiopka]

The [must_use] property on XPIDL methods and attributes is useful for making sure errors are properly handled.

As a first step, we should add the property to PSM methods and attributes that are already correctly checked everywhere.

Comment 1

[:Cykesiopka]

Attachment: Bug 1366584 - Add initial [must_use] properties to PSM IDL files.

The [must_use] property on XPIDL methods and attributes is useful for making
sure errors are properly handled.

As a first step, this patch adds the property to PSM methods and attributes that
are already correctly checked everywhere.

Review commit: https://reviewboard.mozilla.org/r/142472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142472/

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8870899 [details]
Bug 1366584 - Add initial [must_use] properties to PSM IDL files.

https://reviewboard.mozilla.org/r/142472/#review146094

Cool - looks good to me.

::: security/manager/ssl/nsICertificateDialogs.idl:23
(Diff revision 1)
>     *  Provides user with ability to choose trust settings for the cert.
>     *  Asks the user to grant permission to import the certificate.
>     *
>     *  @param ctx A user interface context.
>     *  @param cert The certificate that is about to get installed.
>     *  @param trust a bit mask of trust flags, 

If you feel like it, this might be a good opportunity to remove all of the trailing whitespace in this file, but not a big deal either way.

::: security/manager/ssl/nsICryptoHMAC.idl:49
(Diff revision 1)
>       * WARNING: This approach is not FIPS compliant.
>       *
>       * @throws NS_ERROR_INVALID_ARG if an unsupported algorithm
>       *        type is passed.
>       *
>       * NOTE: This method must be called before any other method 

Same with this file and some others.

::: security/manager/ssl/nsISiteSecurityService.idl:34
(Diff revision 1)
>  [scriptable, uuid(31313372-842c-4110-bdf1-6aea17c845ad), builtinclass]
>  interface nsISiteSecurityState : nsISupports
>  {
> +  [must_use]
>    readonly attribute ACString hostname;
>    [infallible] readonly attribute long long expireTime;

Hmmm - for some attributes it might make sense to use infallible instead, but maybe we can address that later. Just a thought.

Comment 3

[:Cykesiopka]

Comment on attachment 8870899 [details]
Bug 1366584 - Add initial [must_use] properties to PSM IDL files.

https://reviewboard.mozilla.org/r/142472/#review146094

Thanks for the review!

> If you feel like it, this might be a good opportunity to remove all of the trailing whitespace in this file, but not a big deal either way.

Done for this and the other files.

> Hmmm - for some attributes it might make sense to use infallible instead, but maybe we can address that later. Just a thought.

Yeah, I'll file a follow-up for this.

Comment 4

[:Cykesiopka]

Comment on attachment 8870899 [details]
Bug 1366584 - Add initial [must_use] properties to PSM IDL files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142472/diff/1-2/

Comment 5

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cd4770074d3f4d341642c527c2a82aa315037c0
(OSX 10.10 xpcshell tests are still queued, but it doesn't really matter - the changes here really only affect whether things build or not.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a184b592cfa3
Add initial [must_use] properties to PSM IDL files. r=keeler

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a184b592cfa3