SimplePush: 409 error code for conflicting channelIDs might be redundant?

RESOLVED FIXED in mozilla25

Status

()

Core
DOM: Push Notifications
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: nsm, Assigned: sasklacz)

Tracking

Trunk
mozilla25
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=nsm])

Attachments

(1 attachment)

Considering the premise of UUIDs is to be unique, is this check extra paranoid?

Filing follow up to simplify protocol once bug 822712 lands.
Depends on: 822712
See https://bugzilla.mozilla.org/show_bug.cgi?id=822712#c60 (search for string 'case 409')
Also https://bugzilla.mozilla.org/show_bug.cgi?id=822712#c63
Marked as a good first bug for contributors. It's mainly a matter of removing code from the client.
Whiteboard: [mentor=nsm]
Assignee: nsm.nikhil → nobody
(Assignee)

Comment 4

4 years ago
Where to download the required repo from ? It's somewhere on moz hg ?
Flags: needinfo?(nsm.nikhil)
The required code is already in mozilla-central, so once you have a build [1], you can start hacking.

[1] https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions
Flags: needinfo?(nsm.nikhil)
(Assignee)

Comment 6

4 years ago
Can you assign this ticket to me ? I'm waiting for my MBA to finish the build.
Assignee: nobody → owcarnia
(Assignee)

Comment 7

4 years ago
Created attachment 778519 [details] [diff] [review]
First patch
Attachment #778519 - Flags: review?(nsm.nikhil)

Updated

4 years ago
Component: General → DOM: Push Notifications
Product: Boot2Gecko → Core
Version: unspecified → Trunk
Comment on attachment 778519 [details] [diff] [review]
First patch

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

::: dom/push/src/PushService.jsm
@@ +1134,2 @@
>  
> +    if (reply.status) {

|reply.status| is always present, so you cannot error based on its presence alone. A non-200 value is considered an error.

@@ +1134,4 @@
>  
> +    if (reply.status) {
> +      debug("General failure " + reply.status);
> +      throw { requestID: aPageRecord.requestID, error: reply.error };    

Trailing whitespace.
Attachment #778519 - Flags: review?(nsm.nikhil) → review-
Does the tef server send 409?
Flags: needinfo?(willyaranda)
Nope, we are not sending that statuscode.
Flags: needinfo?(willyaranda)
(Assignee)

Comment 11

4 years ago
In the original version it was :


>    switch (reply.status) {
>      case kERROR_CHID_CONFLICT:
>        ...
>      default:
>        ...
>    }

and the function itself is called when an error occurs (as the name suggests). So we should always just throw the default or add additional check for status different than 200 ?
Comment on attachment 778519 [details] [diff] [review]
First patch

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

Yes, you're right. In that case the code is ok, just take care of the trailing whitespace.
Attachment #778519 - Flags: review- → review+
Keywords: checkin-needed
https://hg.mozilla.org/projects/birch/rev/ed1b011524af

Thanks for the patch, Jakub! One request, please use commit messages that explain what the patch is doing rather than just restating the bug title. Thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed1b011524af
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.