Closed Bug 1246341 Opened 9 years ago Closed 9 years ago

Add code/reason to notification ack/unregister

Categories

(Core :: DOM: Notifications, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: benbangert, Assigned: lina)

References

Details

(Whiteboard: dom-triaged)

Attachments

(3 files)

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).
Priority: -- → P1
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Whiteboard: dom-triaged
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?
Flags: needinfo?(bbangert)
That looks good to me, it's not possible to detect an error easily (uncaught exception in service worker)?
Flags: needinfo?(bbangert)
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?
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
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/
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.
Flags: needinfo?(bbangert)
Attachment #8722609 - Flags: review?(dd.mozilla) → review+
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
Attachment #8723928 - Flags: review?(dd.mozilla) → review+
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.
Flags: needinfo?(bbangert)
Attachment #8723927 - Flags: review?(amarchesini)
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/
Attachment #8723927 - Flags: review?(amarchesini)
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/
Attachment #8722609 - Attachment description: MozReview Request: Bug 1246341 - Include status codes in "ack" and "unregister" requests. r?dragana → MozReview Request: Bug 1246341 - Include status codes in "ack" and "unregister" requests. r=dragana
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/
Attachment #8723928 - Attachment description: MozReview Request: Bug 1246341 - Add a test for push event error reporting. r?dragana → MozReview Request: Bug 1246341 - Add a test for push event error reporting. r=dragana
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.
Attachment #8723927 - Flags: review?(amarchesini) → review+
Depends on: 1258883
Blocks: WADI
Blocks: 1266540
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: