For additional developer resources it'd be useful to know about client errors processing a message. To convey this the notification ack should include a code/reason if the notification had issues (bad decryption) per https://github.com/mozilla-services/autopush/issues/330 The unregister can include a code/reason indicating why the unregister occurred (user drive, quota issue, etc).

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

I've added `code` fields to `unregister` and `ack`, with the following values: * 100: Message delivered to application. * 101: Decryption failure. * 102: Message not delivered for other reasons (message sent to expired subscription, or client error). * 200: Subscription deleted manually (either via `.unsubscribe()` or clearing history). * 201: Unsubscribed after exceeding quota. * 202: Unsubscribed because user revoked permission. Does that look OK, Ben?

That looks good to me, it's not possible to detect an error easily (uncaught exception in service worker)?

Maybe. IIUC, we could have https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/dom/workers/ServiceWorkerEvents.cpp#823-846 call back in to `nsIPushService` to report `event.waitUntil()` rejections for `push` events. I don't know about uncaught exceptions, though. :baku, is it possible for us to detect exceptions that happen in `onpush` handlers? I see we have `WorkerPrivate::ReportError`, but can we actually tell whether the exception came from a particular event handler?

You should be in this situation: https://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerPrivate.cpp#272 This is if the exception is thrown in the event handler.

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

Comment on attachment 8722609 [details] MozReview Request: Bug 1246341 - Include status codes in "ack" and "unregister" requests. r=dragana Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36125/diff/1-2/

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

The latest patches add uncaught exception and `event.waitUntil()` rejection reporting. :baku, can you please review `PushErrorReporter` and the `AppendNativeHandler` usage? It borrows a lot from `WaitUntilHandler`; I just changed it to call back in to the Push service if there's an error. The new `messageId` param acts like a cookie that the Push service passes to `notifyPush{WithData}`, and gets back in `reportDeliveryError`. I also saw some places where we could pass const refs to `Maybe<nsTArray>`, so I fixed them up while I was there. :dragana for everything else.

FYI, :benbangert, this will now send a `{"messageType": "nack", "version": "...", "code": ...}` message if the worker fails to process the message. I don't know if "nack" is the best name for this. Here are the possible status codes: * 301: The `push` handler threw an uncaught exception. * 302: The promise passed to `pushEvent.waitUntil()` rejected with an error (useful if the handler calls `fetch()` or performs other async tasks). * 303: Some other error dispatching the event.

Comment on attachment 8722609 [details] MozReview Request: Bug 1246341 - Include status codes in "ack" and "unregister" requests. r=dragana https://reviewboard.mozilla.org/r/36125/#review33355 Looks good.

Comment on attachment 8723928 [details] MozReview Request: Bug 1246341 - Add a test for push event error reporting. r=dragana https://reviewboard.mozilla.org/r/36795/#review33357

I think that's ok to do for a nack. Filed https://github.com/mozilla-services/autopush/issues/380 to address the server-side of this, as we will actually drop the client if they send a message type we aren't expecting.

Comment on attachment 8723927 [details] MozReview Request: Bug 1246341 - Report push event errors and rejections to the Push service. r?baku https://reviewboard.mozilla.org/r/36793/#review33673 I would like to see this patch again. Thanks. ::: dom/interfaces/push/nsIPushErrorReporter.idl:2 (Diff revision 1) > + Missing headers. ::: dom/interfaces/push/nsIPushErrorReporter.idl:15 (Diff revision 1) > + const uint16_t ACK_DELIVERED = 1; what about value 0 ? ::: dom/push/PushNotifier.cpp:144 (Diff revision 1) > - IPC::Principal(aPrincipal), aData.ref()); > + IPC::Principal(aPrincipal), PromiseFlatString(aMessageId), aData.ref()); why do you need to force a flat string? ::: dom/workers/ServiceWorkerPrivate.cpp:11 (Diff revision 1) > +#include "mozilla/unused.h" move it at the end of this 'mozilla/*' include list. ::: dom/workers/ServiceWorkerPrivate.cpp:589 (Diff revision 1) > + { Add an assertion about which thread this is called. ::: dom/workers/ServiceWorkerPrivate.cpp:595 (Diff revision 1) > + Report(nsIPushErrorReporter::WORKER_UNHANDLED_REJECTION); here too ::: dom/workers/ServiceWorkerPrivate.cpp:601 (Diff revision 1) > + You should validate this aReason to check if it's in the correct range. ::: dom/workers/ServiceWorkerPrivate.cpp:614 (Diff revision 1) > + AssertIsOnMainThread(); What is keeping this object alive? What about if you make it a nsRunnable?

https://reviewboard.mozilla.org/r/36793/#review33691 ::: dom/workers/ServiceWorkerPrivate.cpp:614 (Diff revision 1) > + AssertIsOnMainThread(); Hmm, now I'm confused. I thought passing the object to `NS_NewRunnableMethodWithArg` would keep it alive (looking at https://dxr.mozilla.org/mozilla-central/rev/5e0140b6d11821e0c2a2de25bc5431783f03380a/xpcom/glue/nsThreadUtils.h#305-313, compared to the `MOZ_NON_OWNING_REF` below it), but sounds like that's wrong? If I use `nsRunnable`, do I need to store it in a `RefPtr` member variable? Thanks for the review! I'll post a new patch in a bit.

Comment on attachment 8723927 [details] MozReview Request: Bug 1246341 - Report push event errors and rejections to the Push service. r?baku Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36793/diff/1-2/

Comment on attachment 8722609 [details] MozReview Request: Bug 1246341 - Include status codes in "ack" and "unregister" requests. r=dragana Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36125/diff/2-3/

Comment on attachment 8723928 [details] MozReview Request: Bug 1246341 - Add a test for push event error reporting. r=dragana Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36795/diff/1-2/

https://reviewboard.mozilla.org/r/36793/#review33673 > Missing headers. Added headers to both files. > why do you need to force a flat string? Without it, I get a conversion error (`no known conversion from 'const nsAString_internal' to 'const nsString&'`). Is there a way to avoid that? > You should validate this aReason to check if it's in the correct range. Done! Also renamed `WORKER_` to `DELIVERY_`, since that's the term I use everywhere else. > What is keeping this object alive? > What about if you make it a nsRunnable? Oops, meant to reply to this inline. I miss Splinter's integration with Bugzilla comments. :-/ See comment 15.

https://reviewboard.mozilla.org/r/36125/#review34939 ::: dom/push/PushServiceWebSocket.jsm:943 (Diff revision 3) > + var data = {messageType: 'nack', These should be disabled for the (now-unused) Simple Push backend, where the version is a counter instead of a unique message ID.

Comment on attachment 8723927 [details] MozReview Request: Bug 1246341 - Report push event errors and rejections to the Push service. r?baku https://reviewboard.mozilla.org/r/36793/#review35053 ::: dom/push/PushNotifier.cpp:144 (Diff revision 2) > - IPC::Principal(aPrincipal), aData.ref()); > + IPC::Principal(aPrincipal), PromiseFlatString(aMessageId), aData.ref()); Still not sure why this is needed. ::: dom/workers/ServiceWorkerPrivate.cpp:599 (Diff revision 2) > + you use mWorkerPrivate just for asserting, but this worker can be kept alive just by this promise object. Nullify mWorkerPrivate after this use and be 100% you don't touch it anymore.

https://hg.mozilla.org/integration/mozilla-inbound/rev/69fe21c66f7f https://hg.mozilla.org/integration/mozilla-inbound/rev/7aefa268e50c https://hg.mozilla.org/integration/mozilla-inbound/rev/210366388d90

https://hg.mozilla.org/mozilla-central/rev/69fe21c66f7f https://hg.mozilla.org/mozilla-central/rev/7aefa268e50c https://hg.mozilla.org/mozilla-central/rev/210366388d90

FWIW, the test is failing when making Promises to follow the spec (and use microtask scheduling) https://treeherder.mozilla.org/logviewer.html#?job_id=33320486&repo=try#L2286 I'm trying to figure out why the extra reportDeliveryError call with message-2