1470030 - convert manually-written nsINSSComponent definition to idl

Status: RESOLVED FIXED

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

Description

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

Currently nsINSSComponent is defined in nsNSSComponent.h by hand. This isn't great because a) it involves some magic incantations that nobody currently understands, b) adding a function involves declaring it twice - once in nsINSSComponent and again in nsNSSComponent, and c) we can't add js-accessible APIs to it (in some cases we currently (ab)use the preference service as a mechanism for js to influence nsNSSComponent). If we were to move the definition of nsINSSComponent to idl, we essentially solve all of these problems.

(One reason we may want to add js-accessible APIs to nsINSSComponent is to enable, e.g. UI to trust a specific enterprise/av root certificate. Currently all of the relevant code and state is in nsNSSComponent, so it makes sense to implement the necessary functionality there.)

Comment 1

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

Attachment: bug 1470030 - convert manually-written nsINSSComponent definition to idl

Defining nsINSSComponent in idl rather than manually in a header file allows us
to make full use of the machinery that already exists to process and generate
the correct definitions. Furthermore, it enables us to define JS-accessible APIs
on nsINSSComponent, which enables us to build frontend features that can work
directly with the data and functionality the underlying implementation has
access to.

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

Comment 2

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8986844 [details]
bug 1470030 - convert manually-written nsINSSComponent definition to idl

https://reviewboard.mozilla.org/r/252096/#review258718

lgtm with a couple nits/questions.

::: security/manager/ssl/nsINSSComponent.idl:32
(Diff revision 1)
> +   * and any remembered client authentication certificate decisions, and then
> +   * cancels all network connections (strictly speaking, this last part is
> +   * overzealous - we only need to cancel all https connections (see bug
> +   * 1446645)).
> +   */
> +  [noscript] void logoutAuthenticatedPK11();

So none of these functions should be called from JS?

::: security/manager/ssl/nsINSSComponent.idl:55
(Diff revision 1)
> +   * (i.e. root certificates gleaned from the OS certificate store). Returns
> +   * null otherwise.
> +   * Currently this is only implemented on Windows, so this function returns
> +   * null on all other platforms.
> +   */
> +  [noscript] nsIX509CertList getEnterpriseRoots();

Empty lines between some of these functions would be great for readability :)

::: security/manager/ssl/nsNSSComponent.cpp:2405
(Diff revision 1)
>    return NS_ERROR_FAILURE;
>  }
>  
>  SharedCertVerifier::~SharedCertVerifier() { }
>  
> -already_AddRefed<SharedCertVerifier>
> +nsresult

Shouldn't this be `NS_IMETHODIMP`?

Comment 3

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

Comment on attachment 8986844 [details]
bug 1470030 - convert manually-written nsINSSComponent definition to idl

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

Comment 4

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

Comment on attachment 8986844 [details]
bug 1470030 - convert manually-written nsINSSComponent definition to idl

https://reviewboard.mozilla.org/r/252096/#review258718

Thanks!

> So none of these functions should be called from JS?

Well, at least not until we have a good reason to do so. It's easy to change later if we need to.

> Empty lines between some of these functions would be great for readability :)

Sounds good.

> Shouldn't this be `NS_IMETHODIMP`?

Yes - good catch.

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8539169be58fcc933b253b8fa225fb09f962bb3

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73325580ec50
convert manually-written nsINSSComponent definition to idl r=fkiefer

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/73325580ec50

Comment 8

[Franziskus Kiefer [:franziskus]]

Attachment: Bug 1470030 - follow-up fixing covertiy null-deref issue, r?keeler

Coverity found this issue. We shouldn't continue if n is null because CERT_LIST_NEXT dereferences n.

Comment 9

[Phabricator Automation]

Comment on attachment 8988679 [details]
Bug 1470030 - follow-up fixing covertiy null-deref issue, r?keeler

[:keeler] (use needinfo) has approved the revision.

https://phabricator.services.mozilla.com/D1876

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/330b4ae44da8
follow-up fixing covertiy null-deref issue, r=keeler

Comment 11

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/330b4ae44da8