Closed
Bug 1246341
Opened 8 years ago
Closed 8 years ago
Add code/reason to notification ack/unregister
Categories
(Core :: DOM: Notifications, defect, P1)
Core
DOM: Notifications
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).
Updated•8 years ago
|
Priority: -- → P1
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → kcambridge
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Whiteboard: dom-triaged
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36125/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36125/
Attachment #8722609 -
Flags: review?(dd.mozilla)
Assignee | ||
Comment 2•8 years ago
|
||
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)
Reporter | ||
Comment 3•8 years ago
|
||
That looks good to me, it's not possible to detect an error easily (uncaught exception in service worker)?
Flags: needinfo?(bbangert)
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36793/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36793/
Attachment #8723927 -
Flags: review?(amarchesini)
Assignee | ||
Comment 7•8 years ago
|
||
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/
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36795/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36795/
Attachment #8723928 -
Flags: review?(dd.mozilla)
Assignee | ||
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8722609 -
Flags: review?(dd.mozilla) → review+
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Reporter | ||
Comment 13•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8723927 -
Flags: review?(amarchesini)
Comment 14•8 years ago
|
||
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?
Assignee | ||
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
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
Assignee | ||
Comment 18•8 years ago
|
||
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
Assignee | ||
Comment 19•8 years ago
|
||
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.
Assignee | ||
Comment 20•8 years ago
|
||
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 21•8 years ago
|
||
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+
Comment 22•8 years ago
|
||
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
Assignee | ||
Comment 23•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa5525c8414a
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/69fe21c66f7f https://hg.mozilla.org/mozilla-central/rev/7aefa268e50c https://hg.mozilla.org/mozilla-central/rev/210366388d90
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 25•7 years ago
|
||
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.
Description
•