717869 - New records being created with GUIDs with trailing newlines

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Not sure if this is localDB or Android Sync…

Comment 1

[Richard Newman [:rnewman]]

Example logs:

01-12 23:36:27.195 D/HistoryRecord( 9786): Getting payload for history record zugt3AYmr324 (12).
01-12 23:36:27.200 D/ConcurrentRecordConsumer( 9786): Done with record.
01-12 23:36:27.200 D/ConcurrentRecordConsumer( 9786): Grabbing record...
01-12 23:36:27.200 D/HistoryRecord( 9786): Getting payload for history record uuaNVjdfwPyb
01-12 23:36:27.200 D/HistoryRecord( 9786):  (13).
01-12 23:36:27.205 D/ConcurrentRecordConsumer( 9786): Done with record.
01-12 23:36:27.205 D/ConcurrentRecordConsumer( 9786): Grabbing record...
01-12 23:36:27.205 D/HistoryRecord( 9786): Getting payload for history record EtFOOKJTF6f7
01-12 23:36:27.205 D/HistoryRecord( 9786):  (13).

01-12 23:36:27.485 D/RecordUploadRunnable( 9786): Successful records: ["AcsFnvqFunrz","DmSl5RNa3GpG","4vaEO2b6AA37","HgZ6wPGYEvDr","EFIdCIvEWay0","k05bYeCmp6FU","4hyokRFMjfTa","pcG7vJF0jbCl","fUPQLlbQDmGr","0gRfFJ1Fj3y8","2EuJGqrUPZpN","zugt3AYmr324","uuaNVjdfwPyb\n","EtFOOKJTF6f7\n","63lVQPXhPW80\n"]

Comment 2

[Richard Newman [:rnewman]]

Ah, this is due to:

    public static String generateGuid() {
        byte[] encodedBytes = Base64.encode(generateRandomBytes(9), Base64.URL_SAFE);
        return new String(encodedBytes);
    }

android.util.Base64 appends trailing newlines.

Of interest: URL_SAFE exists since API level 8. I thought we were targeting older than that?

The fix, if we're OK with API level 8+, is to OR that with Base64.NO_WRAP. Patch to follow.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Just like the current code, this requires v8 or higher.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 588442 [details] [diff] [review]
Proposed patch. v1

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

::: mobile/android/base/db/BrowserProvider.java.in
@@ +206,5 @@
>      static final String qualifyColumn(String table, String column) {
>          return table + "." + column;
>      }
>  
> +    private static final int GUID_ENCODE_FLAGS = Base64.URL_SAFE | Base64.NO_WRAP;

are you sure about Base64.URL_SAFE? My previous testing showed this to be incompatible with how Gecko does Base64 encoding.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Brad Lassey [:blassey] from comment #4)

> > +    private static final int GUID_ENCODE_FLAGS = Base64.URL_SAFE | Base64.NO_WRAP;
> 
> are you sure about Base64.URL_SAFE? My previous testing showed this to be
> incompatible with how Gecko does Base64 encoding.

The existing code used URL_SAFE; this patch just adds NO_WRAP to get rid of the newline.

URL_SAFE is equivalent to

  .replace("+", "-").replace("/", "_")

All of Sync's GUIDs, both existing and ones that we generate, are URL-safe.

I don't know if there are parts of Gecko that will barf at [-_] in GUIDs, but I would have thought that they would cause problems in desktop Firefox if that were the case. Do you have a particular instance in mind that would be incompatible?

(CCing Erin, because this is a prerequisite for landing today.)

Comment 6

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/7db237c4472c

Brad, if you folks decide that targeting 8+ is too high, then please file a follow-up; there'll need to be more changes to this file.

Thanks for the quick review!

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #6)
> https://hg.mozilla.org/mozilla-central/rev/7db237c4472c
> 
> Brad, if you folks decide that targeting 8+ is too high, then please file a
> follow-up; there'll need to be more changes to this file.
> 
> Thanks for the quick review!

We target 5, but build against 13. Anything in between needs to be feature tested. I think when it comes to constants this might be okay. Kevin, can you test this on an eclair device?

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

I think we have one working Eclair device in the office. Will check tomorrow.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Richard, please request aurora approval

Comment 10

[Richard Newman [:rnewman]]

Urk, this didn't already make it into Aurora? Oh dear.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 588442 [details] [diff] [review]
Proposed patch. v1

[Approval Request Comment]
User impact if declined: Aurora Fennec generates corrupt history records that will cause valid Sync clients to report an error.
Testing completed (on m-c, etc.): This has been baking for a long time.
Risk to taking this patch (and alternatives if risky): None whatsoever.


Users who have ever used Aurora Fennec must choose "Prefs > Reset Sync > Replace other devices" from a Firefox desktop machine in order to eliminate corrupt records from the server.

Comment 12

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/38777175ae77