Closed
Bug 1205137
Opened 9 years ago
Closed 8 years ago
Add a `PushSubscription` serializer
Categories
(Core :: DOM: Notifications, defect)
Core
DOM: Notifications
Tracking
()
RESOLVED
FIXED
mozilla46
People
(Reporter: lina, Assigned: lina)
References
Details
Attachments
(2 files)
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1205137 - Add a `PushSubscription` serializer. r?mt,nsm
Attachment #8661560 -
Flags: review?(nsm.nikhil)
Attachment #8661560 -
Flags: review?(martin.thomson)
Attachment #8661560 -
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/#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.
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
>(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 4•9 years ago
|
||
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.
Flags: needinfo?(nsm.nikhil)
Assignee | ||
Comment 6•9 years ago
|
||
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.
Assignee | ||
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b70f94a2521
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.
Comment 12•9 years ago
|
||
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
Assignee | ||
Comment 13•9 years ago
|
||
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)
Assignee | ||
Comment 14•9 years ago
|
||
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?
Comment 15•9 years ago
|
||
(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 16•9 years ago
|
||
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 17•9 years ago
|
||
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 18•9 years ago
|
||
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)
Comment 19•9 years ago
|
||
https://reviewboard.mozilla.org/r/19403/#review17613 The interface has a method, so that's not really an option.
Comment 20•9 years ago
|
||
(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 21•9 years ago
|
||
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 22•9 years ago
|
||
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)
Comment 23•9 years ago
|
||
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. ;)
Assignee | ||
Comment 24•8 years ago
|
||
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)
Assignee | ||
Comment 25•8 years ago
|
||
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 26•8 years ago
|
||
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+
Assignee | ||
Comment 27•8 years ago
|
||
Looks like :bz and :bholley are busy. 302 :smaug.
Assignee | ||
Comment 28•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8661560 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8698768 -
Flags: review?(nsm.nikhil)
Comment 30•8 years ago
|
||
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 31•8 years ago
|
||
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+
Comment 32•8 years ago
|
||
Ah, I had reviewed some old version.
Assignee | ||
Comment 33•8 years ago
|
||
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
Assignee | ||
Comment 34•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61b0a0cbae31
Assignee | ||
Comment 35•8 years ago
|
||
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.
Assignee | ||
Comment 36•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/dda92c46bb23ea846ca288da9cf9e102347699d6 Bug 1205137 - Add a `PushSubscription` serializer. r=mt,smaug
Comment 37•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dda92c46bb23
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 38•8 years ago
|
||
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+
Comment 39•5 years ago
|
||
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.
Description
•