GCM: Fix for null Sender ID on Android

GCM: Fix for null Sender ID on Android

ChromeGcmListenerService was reading the sender ID from the
wrong place (possibly because GCM used to include the
sender ID both as a method parameter and in the bundle,
then later stopped including it in the Bundle). This patch
fixes that.

The only real consequence of a missing sender ID was that
if PushMessagingServiceImpl::DeliverMessageCallback decided
to unsubscribe the subscription due to an error,
PushMessagingServiceImpl::Unsubscribe would hit the
sender_id.empty() code path and fail to unsubscribe from
GCM (but messages would already have stopped being
delivered).

Even that consequence will become irrelevant once we switch
to InstanceID, which no longer requires sender ID in order
to unsubscribe.

BUG=599434
Committed: https://crrev.com/598e5bd98f9199a001cfdaa37842e40564538bed
Cr-Commit-Position: refs/heads/master@{#384943}

[johnme, 2016-04-04 14:30:35]

johnme@chromium.org changed reviewers:
+ peter@chromium.org

[johnme, 2016-04-04 14:34:04]

Description was changed from

==========
Null check for sender ID

Fixes DCHECK when GCM omits the sender ID (which shouldn't normally
happen, but sometimes does).

BUG=599434
==========

to

==========
Null check for sender ID

Fixes DCHECK when GCM omits the sender ID (which shouldn't normally
happen, but sometimes does).

The only real consequence of a missing sender ID is that if
PushMessagingServiceImpl::DeliverMessageCallback decides to unsubscribe the
subscription due to an error, PushMessagingServiceImpl::Unsubscribe will hit the
sender_id.empty() code path which cleanly fails to unsubscribe.

BUG=599434
==========

[johnme, 2016-04-04 14:34:46]

Description was changed from

==========
Null check for sender ID

Fixes DCHECK when GCM omits the sender ID (which shouldn't normally
happen, but sometimes does).

The only real consequence of a missing sender ID is that if
PushMessagingServiceImpl::DeliverMessageCallback decides to unsubscribe the
subscription due to an error, PushMessagingServiceImpl::Unsubscribe will hit the
sender_id.empty() code path which cleanly fails to unsubscribe.

BUG=599434
==========

to

==========
Null check for sender ID

Fixes DCHECK when GCM omits the sender ID (which shouldn't normally
happen, but sometimes does).

The only real consequence of a missing sender ID is that if
PushMessagingServiceImpl::DeliverMessageCallback decides to unsubscribe the
subscription due to an error, PushMessagingServiceImpl::Unsubscribe will hit the
sender_id.empty() code path which cleanly fails to unsubscribe.

Even this will become irrelevant once we switch to InstanceID, which no longer
requires sender ID in order to unsubscribe.

BUG=599434
==========

[Peter Beverloo, 2016-04-04 14:41:21]

This would happen when someone uses Stubby to send messages but doesn't fill in
the sender Id. It's a potential GCM issue, I'll file a bug.

Please annotate this in the Java file as well. A one line comment would suffice.

https://code.google.com/p/chromium/codesearch#chromium/src/components/gcm_dri...

lgtm

[Peter Beverloo, 2016-04-04 14:43:22]

I just read your reply on the bug, that's more sketchy and definitely not
expected.

I'm OK landing this as-is (w/ the comment), but this really should be a DCHECK
as it breaks functionality. Let's start a thread.

[johnme, 2016-04-04 15:53:43]

Description was changed from

==========
Null check for sender ID

Fixes DCHECK when GCM omits the sender ID (which shouldn't normally
happen, but sometimes does).

The only real consequence of a missing sender ID is that if
PushMessagingServiceImpl::DeliverMessageCallback decides to unsubscribe the
subscription due to an error, PushMessagingServiceImpl::Unsubscribe will hit the
sender_id.empty() code path which cleanly fails to unsubscribe.

Even this will become irrelevant once we switch to InstanceID, which no longer
requires sender ID in order to unsubscribe.

BUG=599434
==========

to

==========
GCM: Fix for null Sender ID on Android

ChromeGcmListenerService was reading the sender ID from the
wrong place (possibly because GCM used to include the
sender ID both as a method parameter and in the bundle,
then later stopped including it in the Bundle). This patch
fixes that.

The only real consequence of a missing sender ID was that
if PushMessagingServiceImpl::DeliverMessageCallback decided
to unsubscribe the subscription due to an error,
PushMessagingServiceImpl::Unsubscribe would hit the
sender_id.empty() code path and fail to unsubscribe from
GCM (but messages would already have stopped being
delivered).

Even that consequence will become irrelevant once we switch
to InstanceID, which no longer requires sender ID in order
to unsubscribe.

BUG=599434
==========

[johnme, 2016-04-04 15:53:58]

Total re-write. PTAL.

[johnme, 2016-04-04 16:42:14]

Peter renewed his lgtm offline.

[johnme, 2016-04-04 16:42:19]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-04-04 16:42:19]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1856753002/#ps40001
(title: "Fix ChromeGcmListenerService instead of adding null check")

[commit-bot: I haz the power, 2016-04-04 16:42:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1856753002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1856753002/40001

[commit-bot: I haz the power, 2016-04-04 16:57:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-04 16:57:06]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[johnme, 2016-04-04 17:05:35]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-04-04 17:05:35]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1856753002/#ps60001
(title: "Remove @Nullable")

[commit-bot: I haz the power, 2016-04-04 17:05:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1856753002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1856753002/60001

[commit-bot: I haz the power, 2016-04-04 17:50:27]

Description was changed from

==========
GCM: Fix for null Sender ID on Android

ChromeGcmListenerService was reading the sender ID from the
wrong place (possibly because GCM used to include the
sender ID both as a method parameter and in the bundle,
then later stopped including it in the Bundle). This patch
fixes that.

The only real consequence of a missing sender ID was that
if PushMessagingServiceImpl::DeliverMessageCallback decided
to unsubscribe the subscription due to an error,
PushMessagingServiceImpl::Unsubscribe would hit the
sender_id.empty() code path and fail to unsubscribe from
GCM (but messages would already have stopped being
delivered).

Even that consequence will become irrelevant once we switch
to InstanceID, which no longer requires sender ID in order
to unsubscribe.

BUG=599434
==========

to

==========
GCM: Fix for null Sender ID on Android

ChromeGcmListenerService was reading the sender ID from the
wrong place (possibly because GCM used to include the
sender ID both as a method parameter and in the bundle,
then later stopped including it in the Bundle). This patch
fixes that.

The only real consequence of a missing sender ID was that
if PushMessagingServiceImpl::DeliverMessageCallback decided
to unsubscribe the subscription due to an error,
PushMessagingServiceImpl::Unsubscribe would hit the
sender_id.empty() code path and fail to unsubscribe from
GCM (but messages would already have stopped being
delivered).

Even that consequence will become irrelevant once we switch
to InstanceID, which no longer requires sender ID in order
to unsubscribe.

BUG=599434
==========

[commit-bot: I haz the power, 2016-04-04 17:50:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-04 17:52:00]

Description was changed from

==========
GCM: Fix for null Sender ID on Android

ChromeGcmListenerService was reading the sender ID from the
wrong place (possibly because GCM used to include the
sender ID both as a method parameter and in the bundle,
then later stopped including it in the Bundle). This patch
fixes that.

The only real consequence of a missing sender ID was that
if PushMessagingServiceImpl::DeliverMessageCallback decided
to unsubscribe the subscription due to an error,
PushMessagingServiceImpl::Unsubscribe would hit the
sender_id.empty() code path and fail to unsubscribe from
GCM (but messages would already have stopped being
delivered).

Even that consequence will become irrelevant once we switch
to InstanceID, which no longer requires sender ID in order
to unsubscribe.

BUG=599434
==========

to

==========
GCM: Fix for null Sender ID on Android

ChromeGcmListenerService was reading the sender ID from the
wrong place (possibly because GCM used to include the
sender ID both as a method parameter and in the bundle,
then later stopped including it in the Bundle). This patch
fixes that.

The only real consequence of a missing sender ID was that
if PushMessagingServiceImpl::DeliverMessageCallback decided
to unsubscribe the subscription due to an error,
PushMessagingServiceImpl::Unsubscribe would hit the
sender_id.empty() code path and fail to unsubscribe from
GCM (but messages would already have stopped being
delivered).

Even that consequence will become irrelevant once we switch
to InstanceID, which no longer requires sender ID in order
to unsubscribe.

BUG=599434

Committed: https://crrev.com/598e5bd98f9199a001cfdaa37842e40564538bed
Cr-Commit-Position: refs/heads/master@{#384943}
==========

[commit-bot: I haz the power, 2016-04-04 17:52:01]

Patchset 4 (id:??) landed as
https://crrev.com/598e5bd98f9199a001cfdaa37842e40564538bed
Cr-Commit-Position: refs/heads/master@{#384943}

[Peter Beverloo, 2016-04-04 19:21:29]

lgtm