Closed Bug 1149195 Opened 8 years ago Closed 8 years ago

Implement PushMessageData methods

Categories

(Core :: DOM: Push Notifications, defect)

Product:
Core
Component:
DOM: Push Notifications
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: dougt, Assigned: lina)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

0002-Bug-1149195-Expose-push-message-data-accessors.patch
44.10 KB, patch
dragana
: feedback+
nsm
: feedback+
Details | Diff | Splinter Review
MozReview Request: Bug 1149195 - Expose push message data accessors. r?nsm,dragana
40 bytes, text/x-review-board-request
dragana
: review+
nsm
: review+
smaug
: review+
Details 
interface PushMessageData
{
    ArrayBuffer arrayBuffer();
    Blob        blob();
    object      json();
    USVString   text();
};


Currently, we only have implemented text because I'm lazy.
Assignee: nobody → dougt
Blocks: 1153499
QA Contact: dougt
Blocking on message encryption support.
Depends on: 1172502
We just staged a version of the Push server that supports data. If you apply bug 1172502, bug 1185544, and this, you can send data end-to-end.

Dragana, Nikhil...if you have time, I'd love your feedback. I cargo-culted some C++ to make this work, but I don't really know what I'm doing. :-)
Attachment #8654272 - Flags: feedback?(nsm.nikhil)
Attachment #8654272 - Flags: feedback?(dd.mozilla)
Depends on: 1185544
Comment on attachment 8654272 [details] [diff] [review]
0002-Bug-1149195-Expose-push-message-data-accessors.patch

Review of attachment 8654272 [details] [diff] [review]:
-----------------------------------------------------------------

Nice job Kit! I've only skimmed it, but seems like a reasonable approach. Have you taken a look at how much of FetchBody<Derived>::ContinueConsumeBody and your conversion methods could be refactored?
Attachment #8654272 - Flags: feedback?(nsm.nikhil) → feedback+
Assignee: dougt → nobody
Comment on attachment 8654272 [details] [diff] [review]
0002-Bug-1149195-Expose-push-message-data-accessors.patch

Review of attachment 8654272 [details] [diff] [review]:
-----------------------------------------------------------------

looks ok.

::: dom/push/PushServiceWebSocket.jsm
@@ +218,5 @@
>              serverURL + ")");
>        return null;
>      }
>  
> +    if (uri.scheme !== "wss" && uri.scheme != "ws") {

what is the reason for this?

::: dom/workers/ServiceWorkerEvents.cpp
@@ +526,5 @@
>    NS_INTERFACE_MAP_ENTRY(nsISupports)
>  NS_INTERFACE_MAP_END
>  
>  void
> +PushMessageData::Json(JSContext* cx, JS::MutableHandle<JSObject*> aRetval, ErrorResult& aRv)

check for line length 80.
Attachment #8654272 - Flags: feedback?(dd.mozilla) → feedback+
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Bug 1149195 - Expose push message data accessors. r?nsm,dragana
Attachment #8661500 - Flags: review?(nsm.nikhil)
Attachment #8661500 - Flags: review?(dd.mozilla)
Comment on attachment 8661500 [details]
MozReview Request: Bug 1149195 - Expose push message data accessors. r?nsm,dragana

https://reviewboard.mozilla.org/r/19377/#review17433

::: dom/fetch/FetchUtil.h:8
(Diff revision 1)
> +#include "mozilla/ErrorResult.h"
> +
> +#include "mozilla/dom/File.h"

Nit: Remove the newline.

::: dom/fetch/FetchUtil.h:31
(Diff revision 1)
> +  ConsumeArrayBuffer(JSContext* aCx, JS::MutableHandle<JSObject*> aValue,

All of these need comments explaining how they influence ownership or lifetime of aResult.

::: dom/fetch/FetchUtil.cpp:5
(Diff revision 1)
> +#include "nsIUnicodeDecoder.h"

Nit: Move this in alphabetically with nsError and nsString, and while you are here can you put a newline between FetchUtil and nsError.

::: dom/interfaces/base/nsIServiceWorkerManager.idl:146
(Diff revision 1)
> -                     in DOMString aData);
> +                     in uint32_t aDataLength,
> +                     [array, size_is(aDataLength)] in uint8_t aDataBytes);

You'll have to bump the interface's UUID.

::: dom/push/PushService.jsm:796
(Diff revision 1)
>        return (cryptoParams ? PushCrypto.decodeMsg(
>          message,
>          record.p256dhPrivateKey,
>          cryptoParams.dh,
>          cryptoParams.salt,
>          cryptoParams.rs
> -      ).then(bytes => new TextDecoder("utf-8").decode(bytes)) : Promise.resolve("")).then(message => {
> +      ) : Promise.resolve(null)).then(message => {

While you are here, this is super hard to read. Want to make it:

    let decodedPromise;
    if (cryptoParams) {
      decodedPromise = ...
    } else {
      decodedPromise = Promise.resolve(null);
    }
    
    decodedPromise.then(...)

::: dom/push/test/frame.html:13
(Diff revision 1)
> -            parent.ok(e.data.okay == "yes", "Got a push message.");
> +            (e.data.okay == "yes" ? res : rej)(e.data);
> -            res(pushSubscription);

I'm not sure why you are resolving the promise with the very data the message event sent. Do you want pushSubscription?

You don't seem to be calling parent.ok anymore. Ignore this if you handle it based on the promise, but there is a lot of test code to wade through.

::: dom/push/test/push-server.sjs:5
(Diff revision 1)
> +function concatArrays(arrays, size) {

concatUint8Arrays

::: dom/push/test/test_data.html:180
(Diff revision 1)
> +    ["dom.push.ws.data.enabled", true],

Out of curiosity, can we remove this pref at some point and if so is there a bug for it?

::: dom/push/test/test_data.html:182
(Diff revision 1)
> +    ["dom.push.debug", true],

This will add too much noise on infra, please remove if you don't need it.

::: dom/push/test/webpush.js:1
(Diff revision 1)
> +/*
> + * Browser-based Web Push client for the application server piece.
> + *
> + * Uses the WebCrypto API.
> + * Uses the fetch API.  Polyfill: https://github.com/github/fetch
> + */
> +

You'll have to get Martin to state on the bug that he is ok with you relicensing this code under the MPL (currently MIT), unless he wants to public domain it (usually our test code is public domain).
In either case, put a notice of the license here.

If there is any confusion :gerv might be a good person to ask.

::: dom/push/test/worker.js:7
(Diff revision 1)
> +// importScripts("/tests/dom/push/test/webpush.js");

? Are you sure the test path below is getting exercised?

::: dom/webidl/PushEvent.webidl:14
(Diff revision 1)
> -  // FIXME(nsm): Bug 1149195.
> +  readonly attribute PushMessageData data;

You'll need jst (in Taipei this week) or someone else to sign off on this file.

::: dom/webidl/PushMessageData.webidl:14
(Diff revision 1)
> -    // FIXME(nsm): Bug 1149195.
> -    // These methods will be exposed once encryption is supported.
> -    // ArrayBuffer arrayBuffer();
> -    // Blob        blob();
> +    [Throws]
> +    ArrayBuffer arrayBuffer();
> +    [Throws]
> +    Blob        blob();

Please file a spec issue to make these throwable. I think text() should be throwable for consistency's sake, but i'm not tied to that.

::: dom/workers/ServiceWorkerEvents.h:192
(Diff revision 1)
> -  explicit PushMessageData(const nsAString& aData);
> +  explicit PushMessageData(nsISupports* aOwner, const nsTArray<uint8_t>& aBytes);

Nit: No longer needs to be explicit.

::: dom/workers/ServiceWorkerEvents.cpp:30
(Diff revision 1)
> +#include "mozilla/dom/TypedArray.h"
> +#include "mozilla/dom/EncodingUtils.h"
> +#include "mozilla/dom/FetchUtil.h"
> +#include "nsIUnicodeEncoder.h"
> +#include "nsIUnicodeDecoder.h"

Nit: Alphabetical while keeping mozilla/dom and nsI separate.

::: dom/workers/ServiceWorkerEvents.cpp:448
(Diff revision 1)
> +ExtractBytesFromArrayBufferView(const ArrayBufferView& aView, nsTArray<uint8_t>& aBytes)

In this and the others can you assert that aBytes is empty at the beginning.

::: dom/workers/ServiceWorkerEvents.cpp:451
(Diff revision 1)
> +  aBytes.SetLength(aView.Length());
> +  aBytes.ReplaceElementsAt(0, aBytes.Length(), aView.Data(), aView.Length());

Can this be replaced by InsertElementsAt() here and in the next couple of uses? InsertElementsAt() does almost exactly this.

::: dom/workers/ServiceWorkerEvents.cpp:520
(Diff revision 1)
>  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(PushMessageData);

mOwner needs to be expressed in the CC graph. `s/_0.*/(PushMessageData, mOwner)/`

Nit: No semicolon at the end of the CC macro.

::: dom/workers/ServiceWorkerEvents.cpp:534
(Diff revision 1)
> -  //todo bug 1149195.  Don't be lazy.
> -   NS_ABORT();
> +  if (mBytes.IsEmpty()) {
> +    aRv.Throw(NS_ERROR_INVALID_ARG);

JSON.parse("") raises a SyntaxError. We should have the same behaviour. In fact skipping this check and letting our JSON parser do it will lead to a better error message.

::: dom/workers/ServiceWorkerEvents.cpp:579
(Diff revision 1)
> +PushMessageData::DecodeText()

Call this SetDecodedText() or EnsureDecodedText() since it modifies global state.

::: dom/workers/ServiceWorkerEvents.cpp:594
(Diff revision 1)
> -PushMessageData::Blob()
> +PushMessageData::Contents()

Call this GetContentCopy()

::: dom/workers/ServiceWorkerManager.cpp:2325
(Diff revision 1)
> -    // pei.mData.Construct(mData);
> +      Uint8Array::Create(aCx, mData.Length(), mData.Elements())

This can OOM fail, so it would be good to check for null and bail.

::: dom/workers/ServiceWorkerManager.cpp:2416
(Diff revision 1)
> +  if (!data.SetCapacity(aDataLength, fallible)) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  if (!data.InsertElementsAt(0, aDataBytes, aDataLength, fallible)) {

InsertElementsAt resizes internally, you can skip the resize.
Comment on attachment 8661500 [details]
MozReview Request: Bug 1149195 - Expose push message data accessors. r?nsm,dragana

Bug 1149195 - Expose push message data accessors. r?nsm,dragana
Attachment #8661500 - Flags: review?(nsm.nikhil)
https://reviewboard.mozilla.org/r/19377/#review17433

> Please file a spec issue to make these throwable. I think text() should be throwable for consistency's sake, but i'm not tied to that.

https://github.com/w3c/push-api/issues/165
Olli, could you review the WebIDL change, please? Happy to upload a patch if it's easier than MozReview.
Flags: needinfo?(bugs)
Martin, are you OK with this? I checked in your Web Push client (https://github.com/martinthomson/webpush-client) for the tests.

(In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #7)
> Comment on attachment 8661500 [details]
> You'll have to get Martin to state on the bug that he is ok with you
> relicensing this code under the MPL (currently MIT), unless he wants to
> public domain it (usually our test code is public domain).
> In either case, put a notice of the license here.
Flags: needinfo?(martin.thomson)
patches are always easier than MozReview ;) But looking the mozreview.
Comment on attachment 8661500 [details]
MozReview Request: Bug 1149195 - Expose push message data accessors. r?nsm,dragana

ok, so PushEvent has different kind of eventInit->event properties mapping than
anything else in the platform. A bit weird but service workers is so full of weirdness that I guess this is fine.


Why do we need [Throws] in PushMessageData arrayBuffer() and blob() ? Per spec those methods shouldn't throw. Explain and either remove [Throws] or file spec bugs.
Flags: needinfo?(bugs)
Attachment #8661500 - Flags: review+
(In reply to Olli Pettay [:smaug] from comment #13)
> patches are always easier than MozReview ;) But looking the mozreview.

Olli, now mozreview shows the hg commands you can run to either import the diff or pull the diff locally so you can review it the way you want.
I tend to just click the 'download diff' link :)
(In reply to Olli Pettay [:smaug] from comment #14)
> 
> Why do we need [Throws] in PushMessageData arrayBuffer() and blob() ? Per
> spec those methods shouldn't throw. Explain and either remove [Throws] or
> file spec bugs.

https://github.com/w3c/push-api/issues/165
Comment on attachment 8661500 [details]
MozReview Request: Bug 1149195 - Expose push message data accessors. r?nsm,dragana

https://reviewboard.mozilla.org/r/19377/#review17569

r=me with some important changes.

::: dom/fetch/Fetch.cpp
(Diff revision 2)
> -        localPromise->MaybeReject(NS_ERROR_DOM_UNKNOWN_ERR);
> -        return;
> +      return;
> -      }
> +    }
> -
> +    case CONSUME_BLOB: {

Should this return be inside the if (!error.Failed()) block so that on error you hit the rejection at the end of the function?

::: dom/fetch/Fetch.cpp:1439
(Diff revision 2)
> -      decoder.AppendText(reinterpret_cast<char*>(aResult), aResultLength);
> +      FetchUtil::ConsumeText(decoded, aResultLength, aResult);

ConsumeText can error and should be handled here. You will have to nest the rest of the code one level in with:

if (NS_SUCCEEDED(rv)) {
  // ... rest
}

so you can fall through to the MaybeReject case at the end.

::: dom/fetch/Fetch.cpp:1452
(Diff revision 2)
> -  NS_NOTREACHED("Unexpected consume body type");
> +      NS_NOTREACHED("Unexpected consume body type");

Considering the fall through in case of error, is it possible this gets reached when a normal error (json failure?) happens.

Perhaps you should have a break; at the end of every case.

::: dom/fetch/FetchUtil.h:58
(Diff revision 2)
> +  static nsresult
> +  ConsumeText(nsString& aText, uint32_t aResultLength, uint8_t* aResult);

Nit: Conventionally the out param goes to the end.
(In reply to Kit Cambridge [:kitcambridge] from comment #12)
> Martin, are you OK with this? I checked in your Web Push client
> (https://github.com/martinthomson/webpush-client) for the tests.
> 
> (In reply to Nikhil Marathe [:nsm] (please needinfo?) from comment #7)
> > Comment on attachment 8661500 [details]
> > You'll have to get Martin to state on the bug that he is ok with you
> > relicensing this code under the MPL (currently MIT), unless he wants to
> > public domain it (usually our test code is public domain).
> > In either case, put a notice of the license here.

You may license the webpush-client code as public domain.
Flags: needinfo?(martin.thomson)
https://reviewboard.mozilla.org/r/19375/#review17685

looks good

::: dom/fetch/FetchUtil.h:35
(Diff revision 2)
> +  ConsumeArrayBuffer(JSContext* aCx, JS::MutableHandle<JSObject*> aValue,

This could be in separate class with a neutral name, because it is use with Push as well. But I am not a dom peer.

::: dom/fetch/FetchUtil.h:59
(Diff revision 2)
> +  ConsumeText(nsString& aText, uint32_t aResultLength, uint8_t* aResult);

Variable aResult sounds like it is the return value but it is actually input. (the same in 2-3 functions above)
Comment on attachment 8661500 [details]
MozReview Request: Bug 1149195 - Expose push message data accessors. r?nsm,dragana

https://reviewboard.mozilla.org/r/19377/#review17565

::: dom/fetch/FetchUtil.h:35
(Diff revision 2)
> +  ConsumeArrayBuffer(JSContext* aCx, JS::MutableHandle<JSObject*> aValue,

I would prefer to have this in a separate class with a neutral name. It is use by Fetch and Push now. (but I am not a dom peer)

::: dom/push/PushService.jsm:836
(Diff revision 2)
> +                  new Uint8Array(message.buffer) : message;

you can move this just before it is used first time.
Attachment #8661500 - Flags: review?(dd.mozilla) → review+

I did a review yesterday, but wanted to look at it once again today, and somehow they got mixed up :)
https://hg.mozilla.org/mozilla-central/rev/a39aa7c8e1ee
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
(In reply to Marco Castelluccio [:marco] from comment #26)
> The following pages should be updated:
> https://developer.mozilla.org/en-US/docs/Web/API/PushMessageData
> https://developer.mozilla.org/en-US/docs/Web/API/Push_API/Using_the_Push_API

I'm working on updating my push demo now to include some PushMessageData functionality, but I'm running into problems. The code I'm using in my SW looks like this:

var port;

self.addEventListener('push', function(event) {  
  
  var title = 'Yay a message.';  
  var body = 'Subscription has changed.';  
  var icon = 'push-icon.png';  
  var tag = 'push';

  event.waitUntil(  
    self.registration.showNotification(title, {  
      body: body,  
      icon: icon,  
      tag: tag  
    })  

    var messageData = event.data;
    port.postMessage(messageData + '\'s subscription has changed.');
  );
});

self.onmessage = function(e) {
  port = e.ports[0];
}

The message channel has been set up fine - the message is being posted back to the main context successfully - but event.data is equal to null. Am I doing anything wrong here? I'm using the latest nightly.
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #27)
> I'm working on updating my push demo now to include some PushMessageData
> functionality, but I'm running into problems. The code I'm using in my SW
> looks like this:
> 
> 
> The message channel has been set up fine - the message is being posted back
> to the main context successfully - but event.data is equal to null. Am I
> doing anything wrong here? I'm using the latest nightly.

You need to encrypt the message in order for it to arrive, this could be the issue you're running into.
I'm building a library to simplify writing the server: https://github.com/marco-c/web-push
There's an usage example here: https://github.com/marco-c/tictactoe

Basically, the client web page should send both the subscription end point and its public key to the server:
https://github.com/marco-c/tictactoe/blob/29305509bdd4b06f9209d64cc5f93f00cbab42aa/js/tictactoe.js#L23
https://github.com/marco-c/tictactoe/blob/29305509bdd4b06f9209d64cc5f93f00cbab42aa/js/tictactoe.js#L147

The server needs to encrypt the message, or it will not be sent. My example is using the web-push library that abstracts the encryption process:
https://github.com/marco-c/tictactoe/blob/29305509bdd4b06f9209d64cc5f93f00cbab42aa/server.js#L73

If you want to use my library and you need any help, feel free to ping me on IRC.

N.B.: Notifications with payloads aren't supported yet in Chrome, they're working on it: https://code.google.com/p/chromium/issues/detail?id=486040
Chris,

This all looks good.

> Note: As alluded to above, you need some kind of a server component to handle the endpoint/data encryption and send push message requests. In our demo we have put together a quick-and-dirty server using NodeJS.

Note that you can push from other browsers.  I don't think that we need to mention it, as it has some security consequences, but I've found that it can be handy.

I found the links to line numbers to be a little flaky.  I don't know what you want to do about that.  Splitting the demo into multiple files might make the links more stable.

sw.js needs to listen for pushsubscriptionchange so that it can update the server.  <http://w3c.github.io/push-api/index.html#the-pushsubscriptionchange-event>  On the time scale you are operating on, this isn't a huge problem, but you will find that we don't maintain subscriptions indefinitely and not having that aspect documented will cause long-term problems for users of the API.

The updated protocol requires that you use POST rather than PUT.  I'd have changed that, but you can coordinate changes better than I.
(In reply to Martin Thomson [:mt:] from comment #30)
> Chris,
> 
> This all looks good.

Thanks for the review Martin - really appreciated.

> 
> > Note: As alluded to above, you need some kind of a server component to handle the endpoint/data encryption and send push message requests. In our demo we have put together a quick-and-dirty server using NodeJS.
> 
> Note that you can push from other browsers.  I don't think that we need to
> mention it, as it has some security consequences, but I've found that it can
> be handy.

I'm not 100% sure what you mean by this anyway ;-) I'll drop you a mail offline to ask further questions.

> I found the links to line numbers to be a little flaky.  I don't know what
> you want to do about that.  Splitting the demo into multiple files might
> make the links more stable.

Yeesh, you are right - this was as a result of me putting them all in, then committing a bunch of changes. I've fixed them all now. 

It is a shame you can't put persistent anchors into github listings ;-)

> 
> sw.js needs to listen for pushsubscriptionchange so that it can update the
> server. 
> <http://w3c.github.io/push-api/index.html#the-pushsubscriptionchange-event> 
> On the time scale you are operating on, this isn't a huge problem, but you
> will find that we don't maintain subscriptions indefinitely and not having
> that aspect documented will cause long-term problems for users of the API.

Ok, I think I get the idea here. Again, I'll drop you a line about this before I start working on it.

> The updated protocol requires that you use POST rather than PUT.  I'd have
> changed that, but you can coordinate changes better than I.

Ok, cool. I've updated all the instances that talk about this.
Blocks: 1223563
You need to log in before you can comment on or make changes to this bug.