Closed Bug 1470030 Opened 4 years ago Closed 4 years ago

convert manually-written nsINSSComponent definition to idl

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(2 files)

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 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`?
Attachment #8986844 - Flags: review?(franziskuskiefer) → review+
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.
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73325580ec50
convert manually-written nsINSSComponent definition to idl r=fkiefer
https://hg.mozilla.org/mozilla-central/rev/73325580ec50
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Coverity found this issue. We shouldn't continue if n is null because CERT_LIST_NEXT dereferences n.
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
Attachment #8988679 - Flags: review+
Pushed by franziskuskiefer@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/330b4ae44da8
follow-up fixing covertiy null-deref issue, r=keeler
You need to log in before you can comment on or make changes to this bug.