Closed
Bug 1470030
Opened 6 years ago
Closed 6 years ago
convert manually-written nsINSSComponent definition to idl
Categories
(Core :: Security: PSM, enhancement, P1)
Core
Security: PSM
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 hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 4•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 5•6 years ago
|
||
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73325580ec50
convert manually-written nsINSSComponent definition to idl r=fkiefer
Comment 7•6 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Comment 8•6 years ago
|
||
Coverity found this issue. We shouldn't continue if n is null because CERT_LIST_NEXT dereferences n.
Comment 9•6 years ago
|
||
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+
Comment 10•6 years ago
|
||
Pushed by franziskuskiefer@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/330b4ae44da8
follow-up fixing covertiy null-deref issue, r=keeler
Comment 11•6 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•