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

Status: RESOLVED FIXED

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

Description

[Martin Thomson [:mt:]]

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

This might be used to improve tests a little at the same time.

Comment 1

[Martin Thomson [:mt:]]

Attachment: Bug 1312306 - Update expires handling on RTCCertificate to match spec,

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

Comment 2

[Martin Thomson [:mt:]]

Attachment: Don't review, just testing

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

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

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

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

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

Comment 5

[Martin Thomson [:mt:]]

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.

Comment 6

[Martin Thomson [:mt:]]

Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87908/diff/3-4/

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87908/diff/4-5/

Comment 8

[Bobby Holley (:bholley)]

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. :-(

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

NI for question in comment 10.

Comment 12

[Martin Thomson [:mt:]]

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).

Comment 13

[Martin Thomson [:mt:]]

Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87908/diff/5-6/

Comment 14

[Martin Thomson [:mt:]]

Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87908/diff/6-7/

Comment 15

[Boris Zbarsky [:bzbarsky]]

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 16

[Ben Kelly [:bkelly, not reviewing]]

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?

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

> > +  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.

Comment 18

[Martin Thomson [:mt:]]

> 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.

Comment 19

[Martin Thomson [:mt:]]

Comment on attachment 8803765 [details]
Bug 1312306 - Update expires handling on RTCCertificate to match spec,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87908/diff/7-8/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2694e55d5972
Update expires handling on RTCCertificate to match spec, r=bkelly,jib

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2694e55d5972

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa1f25d45d8e
Followup to fix operator error, a=bustage

Comment 23

[Phil Ringnalda (:philor)]

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