Closed Bug 1246341 Opened 4 years ago Closed 4 years ago

Add code/reason to notification ack/unregister

Categories

(Core :: DOM: Push 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
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: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.