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

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Filing follow up to simplify protocol once bug 822712 lands.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=822712#c60 (search for string 'case 409')

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Also https://bugzilla.mozilla.org/show_bug.cgi?id=822712#c63

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Marked as a good first bug for contributors. It's mainly a matter of removing code from the client.

Comment 4

[Jakub Siemiatkowski (:sasklacz)]

Where to download the required repo from ? It's somewhere on moz hg ?

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 6

[Jakub Siemiatkowski (:sasklacz)]

Can you assign this ticket to me ? I'm waiting for my MBA to finish the build.

Comment 7

[Jakub Siemiatkowski (:sasklacz)]

Attachment: First patch

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Does the tef server send 409?

Comment 10

[Guillermo López :willyaranda (probably SLOW response)]

Nope, we are not sending that statuscode.

Comment 11

[Jakub Siemiatkowski (:sasklacz)]

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 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed1b011524af