Closed Bug 1312306 Opened 4 years ago Closed 3 years ago

Treat expires=0 on RTCPeerConnection.generateCertificate() as 0

Categories

(Core :: WebRTC: Signaling, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mt, Assigned: mt)

Details

Attachments

(1 file, 1 obsolete file)

See https://github.com/w3c/webrtc-pc/issues/879

This might be used to improve tests a little at the same time.
Summary: Treat expires=0 as 0 → Treat expires=0 on RTCPeerConnection.generateCertificate() as 0
Attachment #8803766 - Attachment is obsolete: true
Note to DOM reviewer: I have a PR open on the spec here: https://github.com/w3c/webrtc-pc/pull/880/files

The underlying implementation uses WebCrypto, which is a nightmare API that takes an `object` argument.  That means that I need to use `object` here also.  I opted to go with ObjectOrString (aka AlgorithmIdentifier) to make passing data to webcrypto easier.
Assignee: nobody → martin.thomson
Status: NEW → ASSIGNED
Rank: 25
Priority: -- → P2
Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

I don't have cycles for generic DOM stuff right now, sorry. :-(
Attachment #8803765 - Flags: review?(bobbyholley)
Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

https://reviewboard.mozilla.org/r/87908/#review87128

Lgtm.

::: dom/webidl/RTCPeerConnection.webidl:90
(Diff revision 5)
> +  // Note: this takes an AlgorithmIdentifier rather than the specified
> +  // RTCGenerateCertificateOptions so that we can ensure that all the
> +  // attributes that webcrypto needs survive properly.
>    [Throws, StaticClassOverride="mozilla::dom::RTCCertificate"]
>    static Promise<RTCCertificate> generateCertificate (AlgorithmIdentifier keygenAlgorithm);

Doesn't this ugliness need to be reflected in the spec also?
Attachment #8803765 - Flags: review?(jib) → review+
Attachment #8803765 - Flags: review?(bkelly)
Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

https://reviewboard.mozilla.org/r/87906/#review87760

::: dom/webidl/RTCPeerConnection.webidl:92
(Diff revision 5)
>   Constructor (optional RTCConfiguration configuration,
>                optional object? constraints)]
>  interface RTCPeerConnection : EventTarget  {
> +  // Note: this takes an AlgorithmIdentifier rather than the specified
> +  // RTCGenerateCertificateOptions so that we can ensure that all the
> +  // attributes that webcrypto needs survive properly.

Doesn't this mean we still accept `DOMString` here when the spec is being changed not to accept it?

Also, what do you mean by "survive properly"?  Is this a limitation in our binding code?  Have you talked to Boris or Peter to see if its something we can fix?
NI for question in comment 10.
Flags: needinfo?(martin.thomson)
Thanks for the prod.  The conclusion is that the *spec* needs to use AlgorithmIdentifier here for all the same reasons that I added it to gecko.  I'll get to this next week (I've a few deadlines on Monday that will push this aside).

Also, DOMString might be valid if we implement Ed25519 certificates (which is waaay off).
Flags: needinfo?(martin.thomson)
Attachment #8803765 - Flags: review?(bkelly)
Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

https://reviewboard.mozilla.org/r/87908/#review89048

::: dom/media/webrtc/RTCCertificate.cpp:242
(Diff revision 7)
> +  if (!aOptions.IsObject()) {
> +    return EXPIRATION_DEFAULT;
> +  }
> +  JS::RootedValue value(aCx, JS::ObjectValue(*aOptions.GetAsObject()));
> +  if (!expiration.Init(aCx, value)) {
> +    aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);

You want:

    aRv.NoteJSContextException(aCx);
    
since there's an exception on aCx already, placed there by Init().
Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

https://reviewboard.mozilla.org/r/87908/#review89040

Also, is there a WPT test for this addition anywhere?  Can you add one?

r=me with comments addressed.

::: dom/media/webrtc/RTCCertificate.cpp:50
(Diff revision 7)
>  {
>  public:
>    GenerateRTCCertificateTask(nsIGlobalObject* aGlobal, JSContext* aCx,
>                               const ObjectOrString& aAlgorithm,
> -                             const Sequence<nsString>& aKeyUsages)
> +                             const Sequence<nsString>& aKeyUsages,
> +                             PRTime expires)

nit: Current style guidelines and the rest of this file use argument names like `aExpires`.

::: dom/media/webrtc/RTCCertificate.cpp:243
(Diff revision 7)
> +    return EXPIRATION_DEFAULT;
> +  }
> +  JS::RootedValue value(aCx, JS::ObjectValue(*aOptions.GetAsObject()));
> +  if (!expiration.Init(aCx, value)) {
> +    aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +    return 0;

Why throw an error and return 0 here?  It seems this method falls back to the default value on all other errors.

Or alternatively, under what conditions can this fail, but also pass normal webidl binding checking?
Attachment #8803765 - Flags: review?(bkelly) → review+
> > +  JS::RootedValue value(aCx, JS::ObjectValue(*aOptions.GetAsObject()));
> > +  if (!expiration.Init(aCx, value)) {
> > +    aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> > +    return 0;
> 
> Why throw an error and return 0 here?  It seems this method falls back to
> the default value on all other errors.
> 
> Or alternatively, under what conditions can this fail, but also pass normal
> webidl binding checking?

Boris tells me this can happen if the init object has a getter that throws, etc.  So you can ignore my comment here.
> Also, is there a WPT test for this addition anywhere?  Can you add one?

Philip Jägenstedt is working on some of these, this is why I ended up making this change.  Thanks for the comments, I'll fix and land.
Pushed by martin.thomson@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2694e55d5972
Update expires handling on RTCCertificate to match spec, r=bkelly,jib
https://hg.mozilla.org/mozilla-central/rev/2694e55d5972
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.