Closed Bug 1205137 Opened 5 years ago Closed 4 years ago

Add a `PushSubscription` serializer

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox43 --- affected
firefox46 --- fixed

People

(Reporter: Lina, Assigned: Lina)

References

Details

Attachments

(2 files)

Bug 1205137 - Add a `PushSubscription` serializer. r?mt,nsm
Attachment #8661560 - Flags: review?(nsm.nikhil)
Attachment #8661560 - Flags: review?(martin.thomson)
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

https://reviewboard.mozilla.org/r/19403/#review17379

Not r+ mainly for refactoring and explanation about jsonifier -> ToJSON() renaming.

::: dom/push/PushManager.h:106
(Diff revision 1)
> +  ToJSON(JSContext* aCx, JS::MutableHandle<JSObject*> aValue);

Nit: this method can be const. Same for the worker equivalent.

::: dom/push/PushManager.cpp:291
(Diff revision 1)
> +  nsCString string;

nsAutoCString

::: dom/push/PushManager.cpp:302
(Diff revision 1)
> +  string.Trim("=", false, true);

Nit: usual style with boolean parameters is

    Trim("=", false /* aParamName */, true /* aParamName */);
    
for a little more clarity.

::: dom/push/PushManager.cpp:302
(Diff revision 1)
> +  string.Trim("=", false, true);
> +  string.ReplaceChar('+', '-');
> +  string.ReplaceChar('/', '_');

Please add a comment as to why you are doing these operations. If it is something specified by the spec, verbatim sections copied from the spec are best.

::: dom/push/PushManager.cpp:315
(Diff revision 1)
> +                                                       aValue.BeginReading(),
> +                                                       aValue.Length()));

Nit: indentation is slightly off.

::: dom/push/PushManager.cpp:325
(Diff revision 1)
> +void
> +PushSubscription::ToJSON(JSContext* aCx, JS::MutableHandle<JSObject*> aValue)
> +{

Please run these methods by :bholley or :bz once for use of JSAPI. I'm mainly concerned that we are in the right compartment when operating on the values.

::: dom/push/PushManager.cpp:350
(Diff revision 1)
> +void
> +WorkerPushSubscription::ToJSON(JSContext* aCx,
> +                               JS::MutableHandle<JSObject*> aValue)

The two implementations are identical. Any way to refactor them?

::: dom/webidl/PushSubscription.webidl:25
(Diff revision 1)
> -    jsonifier;
> +    object toJSON();

I'm not sure why this isn't `jsonifier` considering convention and the spec says so.
Status: NEW → ASSIGNED
>(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #2)
> I'm not sure why this isn't `jsonifier` considering convention and the spec
> says so.

So, the spec defines a custom serializer that references internal fields...but, AFAICT, `jsonifier` only serializes public attributes. Is there a better way to do this?
Flags: needinfo?(nsm.nikhil)
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

https://reviewboard.mozilla.org/r/19403/#review17425

What Nikhil said, plus the constant time thing.  We do have to be careful about that, even if we can't be sure that application servers are.

::: dom/push/PushManager.cpp:288
(Diff revision 1)
> +nsresult

static

::: dom/push/PushManager.cpp:297
(Diff revision 1)
> +  nsresult rv = Base64Encode(binaryData, string);

https://dxr.mozilla.org/mozilla-central/source/nsprpub/lib/libc/src/base64.c#153 isn't constant time.  That's unfortunate.

Time to write a table-based implementation I think.

::: dom/push/test/test_data.html:100
(Diff revision 1)
> +        "Mismatched hex-encoded key: " + keyName

Not hex, base64
Attachment #8661560 - Flags: review?(martin.thomson)
(In reply to Kit Cambridge [:kitcambridge] from comment #3)
> >(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #2)
> > I'm not sure why this isn't `jsonifier` considering convention and the spec
> > says so.
> 
> So, the spec defines a custom serializer that references internal
> fields...but, AFAICT, `jsonifier` only serializes public attributes. Is
> there a better way to do this?

Looks like ToJSON is ok http://logs.glob.uno/?c=content#c322197
I would still like to see the refactoring.
https://reviewboard.mozilla.org/r/19403/#review17379

> Nit: this method can be const. Same for the worker equivalent.

If I make this `const`, I get linker errors complaining about undefined symbols from ` mozilla::dom::PushSubscriptionBinding::toJSON` and ` mozilla::dom::PushSubscriptionBinding_workers::toJSON`.

> Please add a comment as to why you are doing these operations. If it is something specified by the spec, verbatim sections copied from the spec are best.

This converts standard Base64 to the URL-safe variant. Added a comment.

> The two implementations are identical. Any way to refactor them?

I tried extracting the implementation into a `BasePushSubscription` class, but got the same "undefined symbols" linker error. Going with a separate function instead.
https://reviewboard.mozilla.org/r/19403/#review17425

> static

Why? It's a function in an anonymous namespace, not a class method, and I don't think I need static linkage...

> https://dxr.mozilla.org/mozilla-central/source/nsprpub/lib/libc/src/base64.c#153 isn't constant time.  That's unfortunate.
> 
> Time to write a table-based implementation I think.

I'm using https://dxr.mozilla.org/mozilla-central/source/xpcom/io/Base64.cpp, which looks like it's table-based.
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

Bug 1205137 - Add a `PushSubscription` serializer. r?mt,nsm
Attachment #8661560 - Flags: review?(nsm.nikhil)
Attachment #8661560 - Flags: review?(martin.thomson)
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

https://reviewboard.mozilla.org/r/19403/#review17567
Attachment #8661560 - Flags: review?(nsm.nikhil) → review+
please ask bholley for jsapi review.
https://reviewboard.mozilla.org/r/19403/#review17425

> Why? It's a function in an anonymous namespace, not a class method, and I don't think I need static linkage...

static has a different meaning in this context.

> I'm using https://dxr.mozilla.org/mozilla-central/source/xpcom/io/Base64.cpp, which looks like it's table-based.

Yeah, it looks like the encode is OK.  Note that you are really using https://dxr.mozilla.org/mozilla-central/source/nsprpub/lib/libc/src/base64.c?offset=700#113
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

Bug 1205137 - Add a `PushSubscription` serializer. r=mt,nsm, r?smaug,bholley
Attachment #8661560 - Attachment description: MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r?mt,nsm → MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,nsm, r?smaug,bholley
Attachment #8661560 - Flags: review?(bugs)
Attachment #8661560 - Flags: review?(bobbyholley)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bf147cea14c

Olli, one more WebIDL change, I promise. :-)

Bobby, could you have a look at the JSAPI usage in `SerializePushSubscription`, please?
(In reply to Kit Cambridge [:kitcambridge] from comment #14)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bf147cea14c
> 
> Olli, one more WebIDL change, I promise. :-)
I'm sure you can always promise 'one more' ;)
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

bz is more familiar with toJSON/jsonifier setup. 
At least the .webidl needs some comment why jsonifier isn't used.
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

https://reviewboard.mozilla.org/r/19403/#review17607

::: dom/push/PushManager.cpp:304
(Diff revision 3)
> +  string.ReplaceChar('+', '-');
> +  string.ReplaceChar('/', '_');

This won't be constant time.
Attachment #8661560 - Flags: review?(martin.thomson)
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

https://reviewboard.mozilla.org/r/19403/#review17613

The jsapi usage here looks ok, but wouldn't it be better to use a WebIDL dictionary instead, which would allow this JS-ification to happen automatically?

::: dom/push/PushManager.cpp:330
(Diff revision 3)
> +  JS::RootedObject map(aCx, JS_NewPlainObject(aCx));

Given that this isn't a Map object, I wouldn't call it "map". Call it |obj| or |result|, or something specific to the use-case here.
Attachment #8661560 - Flags: review?(bobbyholley)
https://reviewboard.mozilla.org/r/19403/#review17613

The interface has a method, so that's not really an option.
(In reply to Martin Thomson [:mt:] from comment #19)
> https://reviewboard.mozilla.org/r/19403/#review17613
> 
> The interface has a method, so that's not really an option.

What do you mean, exactly? I see us constructing a dictionary-ish object with string-valued properties using raw JSAPI. I'm suggesting that we define that in WebIDL so that we have a nice easy struct to work with in C++.
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

I thought I had changed the review request.
Attachment #8661560 - Flags: review?(bugs) → review?(bzbarsky)
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

https://reviewboard.mozilla.org/r/19403/#review17735

::: dom/push/PushManager.cpp:302
(Diff revision 3)
> +  // Replace standard Base64 characters with URL-safe equivalents.

Where is this defined?  The pull request says "url-safe base64 encoding" but doesn't say which one... Please raise a spec issue as needed, because as far as I can tell the spec doesn't define behavior here.

::: dom/push/PushManager.cpp:315
(Diff revision 3)
> +  JS::Rooted<JSString*> data(aCx, JS_NewUCStringCopyN(aCx,

This is forcing a string copy.  Please use ToJSValue to produce a JS::Value from aValue instead of doing it manually.  That way it will do a copy-on-write share when that makes sense.

::: dom/push/PushManager.cpp:325
(Diff revision 3)
> +static void

This method leaves dangling exceptions on aCx in failure cases.  That's not OK.  You need to list toJSON as [Throws] in the IDL, and make sure to throw on the ErrorResult if one of the JSAPI calls in this method fails.

::: dom/push/PushManager.cpp:330
(Diff revision 3)
> +  JS::RootedObject map(aCx, JS_NewPlainObject(aCx));

Need a null-check; JS_NewPlainObject can fail.  Same elsewhere here where you do JS_NewSomething.

::: dom/push/PushManager.cpp:336
(Diff revision 3)
> +  JS::RootedObject keys(aCx, JS_NewPlainObject(aCx));

JS::Rooted<JSObject*> and JS::Rooted<JS::Value> is DOM style, not JS::RootedObject/JS::RootedValue.  Other places in here you're following the style; please follow it consistently.

::: dom/push/PushManager.cpp:337
(Diff revision 3)
> +  JS::RootedValue keysValue(aCx, JS::ObjectValue(*keys));

You don't need keysValue.  JS_DefineProperty has an overload taking HandleObject.

I'd like to see an updated patch with the error handling fixed.
Attachment #8661560 - Flags: review?(bzbarsky)
Oh, Bobby is totally right.  Since the set of property names here is fixed, just define a pair of IDL dictionaries that will do all this work for you.  That's the easy way to get sane error handling here.  ;)
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19403/diff/3-4/
Attachment #8661560 - Attachment description: MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,nsm, r?smaug,bholley → MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r?mt,bholley
Attachment #8661560 - Flags: review?(martin.thomson)
Sorry for letting this slide. Updated with Bobby's suggestion to use a pair of dictionaries, and ported Martin's constant-time Base64 encoder to C++.
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

https://reviewboard.mozilla.org/r/19403/#review25029

This looks right.

Note that I've discovered a flaw in the getKey('auth') code that means that it returns null on Workers.  I've a fix for that coming.

::: dom/push/PushManager.cpp:71
(Diff revision 4)
> +// Base64 URL-encodes a byte array in constant time.

I would never make that claim, but this looks like a pretty good attempt.  Perhaps you can say "aiming to be constant time".

::: dom/push/PushManager.cpp:104
(Diff revision 4)
> +                   const nsTArray<uint8_t>& aSekrit)

aAuthSecret

::: dom/push/test/test_data.html:112
(Diff revision 4)
> +    Object.keys(json.keys).forEach(keyName => {

You can't use Object.keys here, please use `["p256dh", "auth"]` instead.  Otherwise, this won't test that the values are present properly.
Attachment #8661560 - Flags: review?(martin.thomson) → review+
Looks like :bz and :bholley are busy. 302 :smaug.
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19403/diff/4-5/
Attachment #8698768 - Flags: review?(nsm.nikhil)
Attachment #8698768 - Flags: review?(martin.thomson)
Attachment #8698768 - Flags: review?(bugs)
Attachment #8661560 - Attachment is obsolete: true
Attachment #8698768 - Flags: review?(nsm.nikhil)
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

https://reviewboard.mozilla.org/r/19403/#review25319

r+ for the .webidl usage, but the spec really needs to be fixed. 
I assume Martin reviewed that Base64URLEncode does something sane, since I can't know what it should do.

::: dom/push/PushManager.cpp:75
(Diff revision 5)
> +{

I haven't found any specification defining what this method actually should do.
Is it about rfc4648 or rfc6920 or ...?

Filed https://github.com/w3c/push-api/issues/177
Based on wikipedia there are few variants of base64 url encoding and Push API spec doesn't define at all what "the URL-safe base64 encoding" refers to.

::: dom/push/PushManager.cpp:116
(Diff revision 5)
> +}

Ok, so we always have p256 and auth set to some value, at least per spec, and calling Construct on both is fine.
Attachment #8661560 - Flags: review+
Comment on attachment 8698768 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r?mt,smaug

I have no idea what mozreview is doing here. I r+'ed and then request turned back to r?.
Attachment #8698768 - Flags: review?(bugs) → review+
Ah, I had reviewed some old version.
Comment on attachment 8661560 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/19403/diff/5-6/
Attachment #8661560 - Attachment description: MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r?mt,bholley → MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug
Attachment #8661560 - Attachment is obsolete: false
https://reviewboard.mozilla.org/r/19403/#review25319

> I haven't found any specification defining what this method actually should do.
> Is it about rfc4648 or rfc6920 or ...?
> 
> Filed https://github.com/w3c/push-api/issues/177
> Based on wikipedia there are few variants of base64 url encoding and Push API spec doesn't define at all what "the URL-safe base64 encoding" refers to.

Thanks for the catch! Added a comment to clarify that this uses RFC 4648, and moved to `Base64.cpp` so `CryptoBuffer` can use it in the future.
https://hg.mozilla.org/mozilla-central/rev/dda92c46bb23
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment on attachment 8698768 [details]
MozReview Request: Bug 1205137 - Add a `PushSubscription` serializer. r?mt,smaug

That's weird.
Attachment #8698768 - Flags: review?(martin.thomson) → review+

Updating BCD right now. Will be reflected on MDN as being supported in Firefox next time it's pulled.

You need to log in before you can comment on or make changes to this bug.