Last Comment Bug 717869 - New records being created with GUIDs with trailing newlines
: New records being created with GUIDs with trailing newlines
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 12
Assigned To: Richard Newman [:rnewman]
:
:
Mentors:
: 718762 (view as bug list)
Depends on: 662891
Blocks: 709339
  Show dependency treegraph
 
Reported: 2012-01-12 23:37 PST by Richard Newman [:rnewman]
Modified: 2012-02-06 11:01 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
11+


Attachments
Proposed patch. v1 (1.20 KB, patch)
2012-01-13 09:39 PST, Richard Newman [:rnewman]
blassey.bugs: review+
doug.turner: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Richard Newman [:rnewman] 2012-01-12 23:37:55 PST
Not sure if this is localDB or Android Sync…
Comment 1 Richard Newman [:rnewman] 2012-01-12 23:43:27 PST
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] 2012-01-13 00:29:05 PST
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] 2012-01-13 09:39:45 PST
Created attachment 588442 [details] [diff] [review]
Proposed patch. v1

Just like the current code, this requires v8 or higher.
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2012-01-13 12:33:30 PST
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] 2012-01-13 12:46:53 PST
(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] 2012-01-14 09:29:01 PST
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?) 2012-01-16 08:12:04 PST
(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 [:kbrosnan] 2012-01-16 09:30:18 PST
I think we have one working Eclair device in the office. Will check tomorrow.
Comment 9 Brad Lassey [:blassey] (use needinfo?) 2012-01-27 23:34:11 PST
Richard, please request aurora approval
Comment 10 Richard Newman [:rnewman] 2012-01-27 23:49:54 PST
Urk, this didn't already make it into Aurora? Oh dear.
Comment 11 Richard Newman [:rnewman] 2012-01-27 23:52:25 PST
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] 2012-01-29 21:23:06 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/38777175ae77
Comment 13 Richard Newman [:rnewman] 2012-01-30 16:32:45 PST
*** Bug 718762 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.