1247685 - Implement `applicationServerKey` for subscription association and expose options

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

See https://github.com/w3c/push-api/pull/182. `PushSubscriptionOptions` picks up a new entry that the Push service can use to authenticate messages sent to that subscription.

It's fairly easy to make the necessary changes to WebIDL and `nsIPushService`, but I don't know how to validate the `BufferSource` ("MUST include a point on the P-256 elliptic curve, encoded in the uncompressed form"). I assume there's an NSS API for that?

Comment 1

[Martin Thomson [:mt:]]

You don't actually need to check the key in practice if you just want this to work.  If the point isn't on the curve, then signatures won't be valid against that public key.  And the server is required to check...

The check is simple once Bug 1245244 lands.

Comment 2

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1247685 - Validate and store app server keys in the Push service. r=mt

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

Comment 3

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1247685 - Send subscription keys to the Push server. r=mt

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

Comment 4

[Lina Butler [:lina]]

Here's a work-in-progress implementation. I'm going to hold off on requesting review until the server is ready.

:jrconlin, flagging you for FYI/feedback, since this is the DOM side of https://github.com/mozilla-services/autopush/issues/326.

Comment 5

[JR Conlin [:jrconlin,:jconlin]]

https://reviewboard.mozilla.org/r/35563/#review33999

r+ aside from TODO resolution.

::: dom/push/PushServiceWebSocket.jsm:982
(Diff revision 1)
> +        // TODO: Include the base64url-encoded key in the payload.

From our offline discussion, my understanding is that this will be sent as "key" in the subscription registration.

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8721048 [details]
MozReview Request: Bug 1247685 - Validate and store app server keys in the Push service. r=mt

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

Comment 7

[Lina Butler [:lina]]

Comment on attachment 8721049 [details]
MozReview Request: Bug 1247685 - Send subscription keys to the Push server. r=mt

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

Comment 8

[Lina Butler [:lina]]

Forwarding errors correctly was probably the most complicated part of this patch. Previously, we just converted failures to `AbortError`; now, we actually need to proxy some through. For expediency, I exposed the error codes to JS, though there's probably a better way...and we can remove those entirely if we convert `PushManager` to C++ per bug 1252660.

Comment 9

[Lina Butler [:lina]]

Martin, I think I'd like to resolve https://github.com/w3c/push-api/issues/183 before requesting DOM peer review. I agree that rejecting with `InvalidStateError` is cleanest; dropping the old subscription and automatically creating a new one with the app server key would be surprising.

It would be nice to get this in for 47. If this and bug 1246341 make it in, we should be able to support the Push Dev Dashboard.

Comment 10

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/35561/#review34051

::: dom/push/PushCrypto.jsm:190
(Diff revision 2)
> +  validateAppServerKey(key) {

This seems like a reasonable workaround for bug 1245244, but I'm not sure if it's correct. Would love an extra pair of eyes.

::: dom/push/PushManager.cpp:871
(Diff revision 2)
> -    new GetSubscriptionRunnable(proxy, mScope, aAction);
> +    new GetSubscriptionRunnable(proxy, mScope, aAction, appServerKey);

I don't think I can pass `aOptions` directly here, since it's created on the worker thread and used on the main one. That made for some interesting assertions the first time around...

::: dom/push/PushRecord.jsm:234
(Diff revision 2)
> +    return this.appServerKey.length === key.length &&

Should this be constant time?

::: dom/push/PushService.jsm:92
(Diff revision 2)
> + * `nsXPCComponents_Exception::HasInstance` when inspected at shutdown.

In PushComponents.js, I originally wrote `let result = error instanceof Components.Exception ? error.result : Cr.NS_ERROR_FAILURE`, but that caused https://dxr.mozilla.org/mozilla-central/rev/a4929411c0aa3ec6b727e2bc2fc050c8199c6573/js/xpconnect/src/XPCComponents.cpp#1709-1710 to assert at shutdown. I didn't dig into it further, but might be good fodder for a separate bug.

Comment 11

[Martin Thomson [:mt:]]

Kit, my backlog is pretty full right now, but I will try to get to this within 24 hours.

Comment 12

[Lina Butler [:lina]]

No worries. The server won't be ready until next week, and I don't want to rush this in if you're busy (especially since there's a corresponding spec change). We can try uplifting to 47, or just let it ride until 48.

Thanks, Martin.

Comment 13

[Martin Thomson [:mt:]]

Comment on attachment 8721048 [details]
MozReview Request: Bug 1247685 - Validate and store app server keys in the Push service. r=mt

https://reviewboard.mozilla.org/r/35561/#review34953

::: dom/push/PushCrypto.jsm:191
(Diff revision 2)
> +    return crypto.subtle.importKey('raw', key, ECDSA_KEY,
> +                                   true, ['verify'])
> +      .then(cryptoKey =>crypto.subtle.exportKey('raw', cryptoKey))

If your intent here is to canonicalize the key, then I don't think you need to worry.  `importKey()` takes a `BufferSource`, as does the option.  So whatever works with `importKey` works for `applicationServerKey`, you can effectively ignore the value returned by `importKey`:

```
return crypto.subtle.importKey('raw', key, { name: 'ECDSA', namedCurve: 'P-256' }, true, ['verify']).then(() => key)
```

::: dom/push/PushCrypto.jsm:193
(Diff revision 2)
> +      .then(cryptoKey =>crypto.subtle.exportKey('raw', cryptoKey))

space after =>

::: dom/push/PushManager.cpp:90
(Diff revision 2)
> +CopyBufferSourceToArray(const OwningArrayBufferViewOrArrayBuffer& aSource,
> +                        nsTArray<uint8_t>& aArray)

I wonder why we don't have something like this in `dom/bindings/BindingsUtils.cpp`.

Since you need DOM peer review for this patch, consider asking the peer if this could be added to the `OwningArrayBufferViewOrArrayBuffer` class.

Also, I wonder if the name of that class might be change to the shorter `BufferSource`.

::: dom/push/PushService.jsm:1060
(Diff revision 2)
> -  _getByPageRecord(pageRecord) {
> +  _getByPageRecord(p*(ageRecord) {

Oops

::: dom/webidl/PushManager.webidl:11
(Diff revision 2)
> +  BufferSource applicationServerKey;

Maybe add boolean userVisibleOnly and comment it out.

Comment 14

[Martin Thomson [:mt:]]

Comment on attachment 8721049 [details]
MozReview Request: Bug 1247685 - Send subscription keys to the Push server. r=mt

https://reviewboard.mozilla.org/r/35563/#review34957

::: dom/push/PushCrypto.jsm:108
(Diff revision 2)
> +  return btoa(string).replace(/\+/g, '-').replace(/\//g, '_');

You need to trim '=' from the end as well.

::: dom/push/test/test_register_key.html:59
(Diff revision 2)
> +    subscribeWithKey(scope, principal, keyLen, key, callback) {

Given that you are mocking the service here, you don't need to test the functionality of the service, just that all those layers of wrapping actually work properly.

To that end, verify that you can produce an error in the service and that that error is properly exposed.  Also ensure that you can produce a "subscription" when needed.  It's not good to try to replicate the actual implementation of the service in a mock.

I would test that:

a. a subscription request with a valid key arrives at the service and the service can produce a "subscription" (this shouldnt need to be anything other than syntactically valid, and you don't need to store anything unless those layers of wrappers demand it, they should only be routing)

b. a subscription request with an obviously invalid key is rejected before reaching the service

c. a subscription request can be rejected by the service

...all for both window and worker, since the wrappers are different for each.

You are almost there.  Just label the tests to align with that plan.

::: dom/push/test/worker.js:77
(Diff revision 2)
>    if (event.data.type == "publicKey") {

Rather than if (event.data.type === "foo"), maybe it's time to convert this to:

```
var handlers = {
  publicKey(args) {
  },
  ...
};
reply(event, handlers[event.data.type](event.data))
```

::: dom/push/test/worker.js:123
(Diff revision 2)
> +  if (event.data.type == "subscribeWithKey") {

I was going to suggest that you just fold this into an existing "subscribe" function, but you don't seem to have one of those...  Do we not already have a test that just subscribes from a worker?

::: dom/push/test/xpcshell/test_service_child.js:48
(Diff revision 2)
> +  let key = [4, 180, 184, 231, 85, 148, 255, 127, 101, 119, 240, 17, 130, 47,
> +             36, 151, 54, 39, 132, 85, 60, 59, 163, 93, 77, 197, 213, 72, 22,
> +             143, 72, 14, 233, 77, 204, 229, 213, 228, 206, 166, 84, 157, 240,
> +             238, 200, 141, 25, 180, 181, 1, 142, 65, 49, 132, 138, 212, 206,
> +             43, 177, 6, 255, 172, 64, 47, 42];

You can pull in web crypto with:
```
Cu.importGlobalProperties(["crypto"]);
```

That might make this easier.

Comment 15

[Lina Butler [:lina]]

One thing I hadn't thought of: if you pass an `ArrayBufferView` as the app server key, the type information will be lost when we call `subscribeWithKey()`. That's OK if we don't expose the key again, but https://github.com/w3c/push-api/pull/187 makes it possible to retrieve the passed key via `pushSubscription.options.applicationServerKey`.

This could be surprising if I pass a `Uint8Array` (for example) when subscribing, but Firefox returns an `ArrayBuffer` if I later retrieve the subscription and look at its options.

I suppose we can hack around this by serializing the passed key into a structured clone buffer ( `nsStructuredCloneContainer::GetDataAsBase64`), and passing that to `subscribeWithKey()` instead. But this will make validation and equality checking harder: we'll either need to add a way for JS to deserialize these buffers (maybe something on `ChromeUtils`)...or implement the entire validation/comparison function in C++, and expose that to PushService.jsm.

This seems like a lot of extra complexity for a relative edge case. Alternatively, we can just canonicalize the key to an `ArrayBuffer`, and make it clear that you won't get a view back when you access `pushSubscription.options.applicationServerKey`.

Any ideas, Martin?

Comment 16

[Martin Thomson [:mt:]]

This is actually a fairly common problem.  More so because typed arrays need to be cloned when the browser takes them (they are mutable).  So in every case, the retrieved array will be a different instance.

Comment 17

[Lina Butler [:lina]]

I wonder, then, if we could define the interface like this in Firefox:

    partial interface PushSubscriptionOptions {
      ArrayBuffer? applicationServerKey;
    };

Since we don't know if it was originally passed as a view of some kind.

Comment 18

[Martin Thomson [:mt:]]

Ahh, you picked up on my mistake.

Yes, it should be:

    interface PushSubscriptionOptions {
      readonly attribute ArrayBuffer? applicationServerKey;
    };

No need for it to be partial here, and you need `readonly attribute` too.

I'll fix PR #187

Comment 19

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/35563/#review36519

::: dom/push/test/test_register_key.html:117
(Diff revision 2)
> +  });
> +
> +  var registration;
> +  add_task(function* start() {
> +    appServerKey = yield generateKey();
> +    registrar.registerFactory(MOCK_PUSH_SERVICE_CID, "Push Service",

Mocking the service like this makes some interesting things happen when this test is run as part of the suite (`mach mochitest dom/push`).

The first `subscribe` call still uses the real service. Also, the mock service isn't torn down at the end of the test, breaking all tests that run after this one. I'll investigate tomorrow.

Comment 20

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1247685 - WebIDL and DOM implementation changes for app server keys. r=mt r?baku

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

Comment 21

[Lina Butler [:lina]]

Comment on attachment 8721048 [details]
MozReview Request: Bug 1247685 - Validate and store app server keys in the Push service. r=mt

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

Comment 22

[Lina Butler [:lina]]

Comment on attachment 8721049 [details]
MozReview Request: Bug 1247685 - Send subscription keys to the Push server. r=mt

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

Comment 23

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/35561/#review34953

> If your intent here is to canonicalize the key, then I don't think you need to worry.  `importKey()` takes a `BufferSource`, as does the option.  So whatever works with `importKey` works for `applicationServerKey`, you can effectively ignore the value returned by `importKey`:
> 
> ```
> return crypto.subtle.importKey('raw', key, { name: 'ECDSA', namedCurve: 'P-256' }, true, ['verify']).then(() => key)
> ```

Indeed! Simplified as you suggested.

> I wonder why we don't have something like this in `dom/bindings/BindingsUtils.cpp`.
> 
> Since you need DOM peer review for this patch, consider asking the peer if this could be added to the `OwningArrayBufferViewOrArrayBuffer` class.
> 
> Also, I wonder if the name of that class might be change to the shorter `BufferSource`.

I see we already have a helper that converts `nsTArray` to a typed array, but not the other way around. Extracting this is a good idea, and I think WebCrypto could use it for `CryptoBuffer`, too. This patch is pretty big already, so I'll save it for a follow-up.

Comment 24

[Lina Butler [:lina]]

Martin, please take another look; I'll address any issues you find and request DOM peer review for part 1 after that.

I haven't had much luck figuring out what's going on with the test. Tried wrapping the `(un)registerFactory` calls in a `setTimeout(..., 0)`, but that didn't help. And, of course, it fails on e10s.

I didn't try this with William's patch from bug 1244816. That might make it easier to install these kinds of mocks.

Comment 25

[Martin Thomson [:mt:]]

Comment on attachment 8731174 [details]
MozReview Request: Bug 1247685 - WebIDL and DOM implementation changes for app server keys. r=mt r?baku

https://reviewboard.mozilla.org/r/40407/#review37173

Maybe hold off on shipping this until the review on the PR is finished.  Names were in contention.

::: dom/push/PushManager.h:78
(Diff revision 1)
> +  bool
> +  UserVisibleOnly() const
> +  {
> +    return false;
> +  };

I don't think that we want this.  Exposing the attribute suggests that we accept it.  We don't.

::: dom/push/PushManager.cpp:449
(Diff revision 1)
>  already_AddRefed<WorkerPushSubscription>
>  WorkerPushSubscription::Constructor(GlobalObject& aGlobal,
> -                                    const nsAString& aEndpoint,
> +                                    const PushSubscriptionInit& aInitDict,
> -                                    const nsAString& aScope,
> -                                    const Nullable<ArrayBuffer>& aP256dhKey,
> -                                    const Nullable<ArrayBuffer>& aAuthSecret,
>                                      ErrorResult& aRv)

It's a shame that this is duplicating so much code.

Comment 26

[Martin Thomson [:mt:]]

Comment on attachment 8721049 [details]
MozReview Request: Bug 1247685 - Send subscription keys to the Push server. r=mt

https://reviewboard.mozilla.org/r/35563/#review37175

::: dom/push/PushServiceWebSocket.jsm:1038
(Diff revision 3)
>      if (action == "register") {
>        let data = {channelID: this._generateID(),
>                    messageType: action};
> +      if (record.appServerKey) {
> +        data.key = ChromeUtils.base64URLEncode(record.appServerKey, {
> +          padded: true,

padded: true?

::: dom/push/test/xpcshell/test_service_child.js:66
(Diff revision 3)
> +      run_next_test();
> +    }
> +  );
> +});
> +
> +add_test(function test_subscribeWithKey_error() {

Maybe move the error case above the success case so that you can be sure that this isn't triggering the "different key" code.

Comment 27

[Lina Butler [:lina]]

(In reply to Martin Thomson [:mt:] from comment #25)
> It's a shame that this is duplicating so much code.

Indeed. :-/ I see khuey is removing worker descriptors, so that might be a good chance to clean this up. Filed bug 1257401.

(In reply to Martin Thomson [:mt:] from comment #26)
> ::: dom/push/PushServiceWebSocket.jsm:1038
> (Diff revision 3)
> >      if (action == "register") {
> >        let data = {channelID: this._generateID(),
> >                    messageType: action};
> > +      if (record.appServerKey) {
> > +        data.key = ChromeUtils.base64URLEncode(record.appServerKey, {
> > +          padded: true,
> 
> padded: true?

Python's Base64 decoder requires padding. If we go with a tri-state for decoding, as you suggested in bug 1256488, we can rename the encoder option to something like `{ appendPadding: true }`.

Comment 28

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/35561/#review34953

> I see we already have a helper that converts `nsTArray` to a typed array, but not the other way around. Extracting this is a good idea, and I think WebCrypto could use it for `CryptoBuffer`, too. This patch is pretty big already, so I'll save it for a follow-up.

I would have thought that the delta would be almost zero, but that's fine.  Give me a bug number (and put it in a comment).

Comment 29

[Martin Thomson [:mt:]]

Comment on attachment 8721048 [details]
MozReview Request: Bug 1247685 - Validate and store app server keys in the Push service. r=mt

https://reviewboard.mozilla.org/r/35561/#review37825

Sorry about missing this, I thought that I was done.

Comment 30

[Lina Butler [:lina]]

(In reply to Martin Thomson [:mt:] from comment #28)
> https://reviewboard.mozilla.org/r/35561/#review34953
> 
> I would have thought that the delta would be almost zero, but that's fine. 
> Give me a bug number (and put it in a comment).

Maybe I'm overcomplicating this, then. If we want a method on the union itself, I think we'll need to add it to the code generator. Alternatively, maybe we could add it dom/bindings/TypedArray.h? The caller would still need to decompose the union, so it feels like that doesn't give us much.

Comment 31

[Martin Thomson [:mt:]]

Ugh, no, you are right.  The union type is generated code.  Nevermind then.  Adding something to TypedArray.h might be the best thing we can do, but it wouldn't be a class method (i.e., the ideal).

Comment 32

[Lina Butler [:lina]]

Comment on attachment 8731174 [details]
MozReview Request: Bug 1247685 - WebIDL and DOM implementation changes for app server keys. r=mt r?baku

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

Comment 33

[Lina Butler [:lina]]

Comment on attachment 8721048 [details]
MozReview Request: Bug 1247685 - Validate and store app server keys in the Push service. r=mt

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

Comment 34

[Lina Butler [:lina]]

Comment on attachment 8721049 [details]
MozReview Request: Bug 1247685 - Send subscription keys to the Push server. r=mt

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

Comment 35

[Lina Butler [:lina]]

baku, the spec changes landed in https://github.com/w3c/push-api/pull/187.

Comment 36

[Lina Butler [:lina]]

Comment on attachment 8731174 [details]
MozReview Request: Bug 1247685 - WebIDL and DOM implementation changes for app server keys. r=mt r?baku

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

Comment 37

[Lina Butler [:lina]]

Comment on attachment 8721048 [details]
MozReview Request: Bug 1247685 - Validate and store app server keys in the Push service. r=mt

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

Comment 38

[Lina Butler [:lina]]

Comment on attachment 8721049 [details]
MozReview Request: Bug 1247685 - Send subscription keys to the Push server. r=mt

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

Comment 39

[Lina Butler [:lina]]

Rebased after de-workerification. That should take care of the duplication.

Comment 40

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/40407/#review40731

::: dom/push/PushUtil.cpp:24
(Diff revision 3)
> +  }
> +  return true;
> +}
> +
> +/* static */ bool
> +PushUtil::CopyBufferSourceToArray(

baku, would it make sense to put these in dom/bindings/BindingUtils.cpp, like Martin suggested?

It would be nice to move these into the code generator, so that it works with `BufferSource` unions, but are these methods good enough for now?

Comment 41

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/40407/#review40733

::: dom/push/PushManager.cpp:1127
(Diff revision 2)
> +    return;
> +  }
> +  JS::Rooted<JSObject*> buffer(aCx, ArrayBuffer::Create(aCx,
> +                               mAppServerKey.Length(),
> +                               mAppServerKey.Elements()));
> +  if (!buffer) {

Note to self: `PushSubscription::GetKey` needs the same treatment; it doesn't currently null-check.

Comment 42

[Andrea Marchesini [:baku]]

Comment on attachment 8731174 [details]
MozReview Request: Bug 1247685 - WebIDL and DOM implementation changes for app server keys. r=mt r?baku

https://reviewboard.mozilla.org/r/40407/#review42595

First of all, sorry for the delay. It's ok. Except this RAII thing.
Can I see it again?

::: dom/push/Push.js:92
(Diff revision 3)
>          this._requestPermission(resolve, permissionDenied);
>        }
>      });
>    },
>  
> -  subscribe: function() {
> +  subscribe: function(options) {

aOptions

::: dom/push/Push.js:246
(Diff revision 3)
>      let keyView = new Uint8Array(key);
>      keyView.set(rawKey);
>      return key;
>    },
> +
> +  _rejectWithError: function(result) {

aError or aResult

::: dom/push/Push.js:251
(Diff revision 3)
> +  _rejectWithError: function(result) {
> +    let error;
> +    switch (result) {
> +      case Cr.NS_ERROR_DOM_PUSH_INVALID_KEY_ERR:
> +        error = new this.pushManager._window.DOMException(
> +          "Invalid raw ECDSA P-256 public key.",

We don't want to localize these strings?

::: dom/push/PushManager.h:89
(Diff revision 3)
>    already_AddRefed<Promise>
>    PerformSubscriptionActionFromWorker(SubscriptionAction aAction,
>                                        ErrorResult& aRv);
>  
>    already_AddRefed<Promise>
> -  Subscribe(ErrorResult& aRv);
> +  PerformSubscriptionActionFromWorker(SubscriptionAction aAction,

strange indentation... can you move all the params align to the same column?

::: dom/push/PushManager.cpp:70
(Diff revision 3)
>    return NS_OK;
>  }
>  
> +// A helper class that frees an `nsIPushSubscription` key buffer when it
> +// goes out of scope.
> +class MOZ_RAII AutoFreeKeyBuffer final {

{ in a new line

::: dom/push/PushManager.cpp:91
(Diff revision 3)
> +CopySubscriptionKeyToArray(nsIPushSubscription* aSubscription,
> +                           const nsAString& aKeyName,
> +                           nsTArray<uint8_t>& aKey)
> +{
> +  uint8_t* keyBuffer = nullptr;
> +  AutoFreeKeyBuffer autoFree(keyBuffer);

This is wrong. What you want is probably a RAII class that takes uint8_t**. In the current implementation

AutoFreeKeyBuffer::mKeyBuffer points to nullptr and it's not going to be updated when keyBuffer is updated.

::: dom/push/PushManager.cpp:557
(Diff revision 3)
> +  return PerformSubscriptionActionFromWorker(aAction, options, aRv);
> +}
> +
> +already_AddRefed<Promise>
> +PushManager::PerformSubscriptionActionFromWorker(
> +  SubscriptionAction aAction, const PushSubscriptionOptionsInit& aOptions,

strange indentation here too.

::: dom/push/PushManager.cpp:579
(Diff revision 3)
>    }
>  
> +  nsTArray<uint8_t> appServerKey;
> +  if (!aOptions.mApplicationServerKey.IsNull()) {
> +    if (!PushUtil::CopyBufferSourceToArray(
> +          aOptions.mApplicationServerKey.Value(), appServerKey) ||

I prefer:

if (!PushUtil::CopyBufferSourceToArray(aOptions.mApplicationServerKey.Value(),
                                    appServerKey) ||
...

if it's < 80chars.

::: dom/push/PushSubscription.cpp:271
(Diff revision 3)
> +
> +  nsTArray<uint8_t> appServerKey;
> +  if (aInitDict.mAppServerKey.WasPassed() &&
> +      !aInitDict.mAppServerKey.Value().IsNull() &&
> +      !PushUtil::CopyBufferSourceToArray(
> +        aInitDict.mAppServerKey.Value().Value(), appServerKey)) {

can you follow the same indentation pattern of line 261 ?

::: dom/push/PushSubscriptionOptions.h:35
(Diff revision 3)
> +  void
> +  GetApplicationServerKey(JSContext* aCx,
> +                          JS::MutableHandle<JSObject*> aKey,
> +                          ErrorResult& aRv);
> +
> +protected:

private. It's a final class.

::: dom/push/PushSubscriptionOptions.cpp:12
(Diff revision 3)
> +#include "mozilla/dom/PushSubscriptionOptionsBinding.h"
> +
> +namespace mozilla {
> +namespace dom {
> +
> +PushSubscriptionOptions::PushSubscriptionOptions(nsIGlobalObject* aGlobal,

strange indentation here too

::: dom/push/PushSubscriptionOptions.cpp:16
(Diff revision 3)
> +
> +PushSubscriptionOptions::PushSubscriptionOptions(nsIGlobalObject* aGlobal,
> +  nsTArray<uint8_t>&& aAppServerKey)
> +  : mGlobal(aGlobal)
> +  , mAppServerKey(Move(aAppServerKey))
> +{}

MOZ_ASSERT(mGlobal)

::: dom/push/PushSubscriptionOptions.cpp:37
(Diff revision 3)
> +  return PushSubscriptionOptionsBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +void
> +PushSubscriptionOptions::GetApplicationServerKey(JSContext* aCx,
> +  JS::MutableHandle<JSObject*> aKey, ErrorResult& aRv)

indentation :)

::: dom/push/PushSubscriptionOptions.cpp:44
(Diff revision 3)
> +  if (mAppServerKey.IsEmpty()) {
> +    aKey.set(nullptr);
> +    return;
> +  }
> +  JS::Rooted<JSObject*> buffer(aCx, ArrayBuffer::Create(aCx,
> +                               mAppServerKey.Length(),

here too. mAppserverKey.Lengtrh() is a param of ArrayBuffer::Create().

::: dom/push/PushSubscriptionOptions.cpp:46
(Diff revision 3)
> +    return;
> +  }
> +  JS::Rooted<JSObject*> buffer(aCx, ArrayBuffer::Create(aCx,
> +                               mAppServerKey.Length(),
> +                               mAppServerKey.Elements()));
> +  if (!buffer) {

NS_WARN_IF

::: dom/push/PushUtil.cpp:15
(Diff revision 3)
> +/* static */ bool
> +PushUtil::CopyArrayBufferToArray(const ArrayBuffer& aBuffer,
> +                                 nsTArray<uint8_t>& aArray)
> +{
> +  aBuffer.ComputeLengthAndData();
> +  if (!aArray.SetLength(aBuffer.Length(), fallible) ||

1. use some NS_WARN_IF.
2. or:

return !aArray.SetLength(aBuffer.Length(), fallible) && 
aArray.ReplaceElementsAt(0, aBuffer.Length(), aBuffer.Data() ...

::: dom/push/PushUtil.cpp:24
(Diff revision 3)
> +  }
> +  return true;
> +}
> +
> +/* static */ bool
> +PushUtil::CopyBufferSourceToArray(

Sure. File a follow up.

::: dom/webidl/PushSubscription.webidl:32
(Diff revision 3)
>    PushSubscriptionKeys keys;
>  };
>  
> +dictionary PushSubscriptionInit
> +{
> +  required USVString endpoint;

no default values?

Comment 43

[Lina Butler [:lina]]

Comment on attachment 8731174 [details]
MozReview Request: Bug 1247685 - WebIDL and DOM implementation changes for app server keys. r=mt r?baku

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

Comment 44

[Lina Butler [:lina]]

Comment on attachment 8721048 [details]
MozReview Request: Bug 1247685 - Validate and store app server keys in the Push service. r=mt

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

Comment 45

[Lina Butler [:lina]]

Comment on attachment 8721049 [details]
MozReview Request: Bug 1247685 - Send subscription keys to the Push server. r=mt

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

Comment 46

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/40407/#review42595

Thanks very much for the review!

> aOptions

I was told we don't use aArgs in new JS code, so I changed the others instead.

> We don't want to localize these strings?

I was wondering about that. I don't think we currently localize https://dxr.mozilla.org/mozilla-central/source/dom/base/domerr.msg, and it looks like other callers (https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/dom/media/PeerConnection.js#398-400) don't localize, either.

> strange indentation... can you move all the params align to the same column?

Oops, sorry! I wasn't sure how to break this up, since `const PushSubscriptionOptionsInit& aOptions` with the indentation lined up would be over 80 chars. I just let it run over in this case.

> This is wrong. What you want is probably a RAII class that takes uint8_t**. In the current implementation
> 
> AutoFreeKeyBuffer::mKeyBuffer points to nullptr and it's not going to be updated when keyBuffer is updated.

Ouch, that would've been a glaring leak. :-P Thanks for the catch!

> I prefer:
> 
> if (!PushUtil::CopyBufferSourceToArray(aOptions.mApplicationServerKey.Value(),
>                                     appServerKey) ||
> ...
> 
> if it's < 80chars.

Fixed by assigning `aOptions.mApplicationServerKey.Value()` to a variable.

> 1. use some NS_WARN_IF.
> 2. or:
> 
> return !aArray.SetLength(aBuffer.Length(), fallible) && 
> aArray.ReplaceElementsAt(0, aBuffer.Length(), aBuffer.Data() ...

I went with the second one.

> no default values?

Only the ChromeConstructor uses this. Should I add defaults for that?

Comment 47

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/40407/#review40733

> Note to self: `PushSubscription::GetKey` needs the same treatment; it doesn't currently null-check.

Fixed by adding `PushUtil::CopyArrayToArrayBuffer`, since we now use this in three places.

Comment 48

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/40407/#review42595

> Sure. File a follow up.

Filed bug 1264508.

Comment 49

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1247685 - Expose `PushSubscriptionOptions` in interface tests. r?baku

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

Comment 50

[Lina Butler [:lina]]

Comment on attachment 8731174 [details]
MozReview Request: Bug 1247685 - WebIDL and DOM implementation changes for app server keys. r=mt r?baku

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

Comment 51

[Lina Butler [:lina]]

Comment on attachment 8721048 [details]
MozReview Request: Bug 1247685 - Validate and store app server keys in the Push service. r=mt

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

Comment 52

[Lina Butler [:lina]]

Comment on attachment 8721049 [details]
MozReview Request: Bug 1247685 - Send subscription keys to the Push server. r=mt

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

Comment 53

[Lina Butler [:lina]]

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

Comment 54

[Andrea Marchesini [:baku]]

Comment on attachment 8741435 [details]
MozReview Request: Bug 1247685 - Expose `PushSubscriptionOptions` in interface tests. r?baku

https://reviewboard.mozilla.org/r/46473/#review44831

Comment 55

[Andrea Marchesini [:baku]]

Comment on attachment 8731174 [details]
MozReview Request: Bug 1247685 - WebIDL and DOM implementation changes for app server keys. r=mt r?baku

https://reviewboard.mozilla.org/r/40407/#review44823

::: dom/push/Push.js:49
(Diff revision 5)
>  
>    QueryInterface : XPCOMUtils.generateQI([Ci.nsIDOMGlobalPropertyInitializer,
>                                            Ci.nsISupportsWeakReference,
>                                            Ci.nsIObserver]),
>  
> -  init: function(aWindow) {
> +  init: function(window) {

why these changes? in gecko cpp code we use aVariable. In JS, sometimes we do, sometimes we don't.

But 'window' is bad name for a variable.

Comment 56

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ce993bd6aec
https://hg.mozilla.org/integration/mozilla-inbound/rev/8067eb75c506
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb75b364b554
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb97c924da7

Comment 57

[Lina Butler [:lina]]

https://reviewboard.mozilla.org/r/40407/#review44823

> why these changes? in gecko cpp code we use aVariable. In JS, sometimes we do, sometimes we don't.
> 
> But 'window' is bad name for a variable.

I think you suggested "aVariable" for the new functions before, but I was told on IRC we don't use aArgs in new JS code, so I renamed this one instead. Changed the name to `win`, which I see is used in `PeerConnection.js` and some of the B2G APIs. Thanks for catching that, and for all the reviews!

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ce993bd6aec
https://hg.mozilla.org/mozilla-central/rev/8067eb75c506
https://hg.mozilla.org/mozilla-central/rev/cb75b364b554
https://hg.mozilla.org/mozilla-central/rev/5bb97c924da7