64-bit token fixes

64-bit token fixes

BUG=453523
Committed: https://crrev.com/7cf6cbe2f59986f6d87b775d2856cfd799b158fb
Cr-Commit-Position: refs/heads/master@{#314023}

[Charlie, 2015-01-29 22:51:23]

ckehoe@chromium.org changed reviewers:
+ rkc@chromium.org

[Charlie, 2015-01-29 22:51:24]

This fixes the two issues listed in the bug:

1. Token lengths are not passed along correctly within the copresence code.

2. Tokens are converted to padded base64, but the server wants unpadded base64.

https://codereview.chromium.org/870053008/diff/1/chrome/browser/resources/whi...
File chrome/browser/resources/whispernet_proxy/js/wrapper.js (right):

https://codereview.chromium.org/870053008/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:89: { token:
bytesToBase64(msg.token), audible: msg.audible }, msg.samples);
Does it matter that this line is now passing back an unpadded token?

[Charlie, 2015-01-30 01:56:44]

ckehoe@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Charlie, 2015-01-30 01:56:45]

R += xiyuan in case you can take a look this evening.

[xiyuan, 2015-01-30 02:16:55]

lgtm

https://codereview.chromium.org/870053008/diff/1/chrome/browser/resources/whi...
File chrome/browser/resources/whispernet_proxy/js/wrapper.js (right):

https://codereview.chromium.org/870053008/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:89: { token:
bytesToBase64(msg.token), audible: msg.audible }, msg.samples);
On 2015/01/29 22:51:24, Charlie wrote:
> Does it matter that this line is now passing back an unpadded token?

This looks okay to me since we uses it as an opaque key.

rkc@, please confirm.

[chromium-reviews, 2015-01-30 05:14:00]

Actually come to think of it, I don't see how the token would play at all
if this were wrong. I can take another look at the code though.

On Thu, Jan 29, 2015, 6:17 PM null <xiyuan@chromium.org> wrote:

> lgtm
>
>
>
>
> https://codereview.chromium.org/870053008/diff/1/chrome/
> browser/resources/whispernet_proxy/js/wrapper.js
> File chrome/browser/resources/whispernet_proxy/js/wrapper.js (right):
>
> https://codereview.chromium.org/870053008/diff/1/chrome/
> browser/resources/whispernet_proxy/js/wrapper.js#newcode89
> chrome/browser/resources/whispernet_proxy/js/wrapper.js:89
>
<https://codereview.chromium.org/870053008/diff/1/chrome/browser/resources/whi...>:
> { token:
> bytesToBase64(msg.token), audible: msg.audible }, msg.samples);
> On 2015/01/29 22:51:24, Charlie wrote:
> > Does it matter that this line is now passing back an unpadded token?
>
> This looks okay to me since we uses it as an opaque key.
>
> rkc@, please confirm.
>
> https://codereview.chromium.org/870053008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rkc, 2015-01-30 18:22:23]

Shouldn't we also be converting the token we get from the server to padded also?
IIRC, we use atob on the token, which expects a padded token.

[chromium-reviews, 2015-01-30 18:46:58]

Probably. However it seems that the implementation checks only if the
padding added is incorrect, not if it's missing entirely:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I'll add a TODO to put in padding. If this were failing, wouldn't we just
never get encoded tokens back?


On Fri Jan 30 2015 at 10:22:26 AM <rkc@chromium.org> wrote:

> Shouldn't we also be converting the token we get from the server to padded
> also?
> IIRC, we use atob on the token, which expects a padded token.
>
> https://codereview.chromium.org/870053008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Charlie, 2015-01-30 20:03:25]

Uploaded the TODO and some other clarifications. It looks like we're using the
unpadded token from the server everywhere, and undoing the "url-safe" conversion
before storing it in the audio_manager. So stripping the padding in the other
direction here should lead to the same encoding.

[rkc, 2015-01-30 20:06:57]

lgtm

[Charlie, 2015-01-30 21:03:54]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2015-01-30 21:05:04]

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

[commit-bot: I haz the power, 2015-01-30 22:06:22]

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

[commit-bot: I haz the power, 2015-01-30 22:06:23]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie, 2015-01-30 22:40:18]

The CQ bit was checked by ckehoe@chromium.org

[commit-bot: I haz the power, 2015-01-30 22:41:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/870053008/80001

[commit-bot: I haz the power, 2015-01-30 23:22:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-30 23:23:25]

Patchset 5 (id:??) landed as
https://crrev.com/7cf6cbe2f59986f6d87b775d2856cfd799b158fb
Cr-Commit-Position: refs/heads/master@{#314023}