New records being created with GUIDs with trailing newlines

RESOLVED FIXED in Firefox 11

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

unspecified
Firefox 12
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, fennec11+)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Not sure if this is localDB or Android Sync…
(Assignee)

Comment 1

6 years ago
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"]
(Assignee)

Comment 2

6 years ago
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.
Component: Android Sync → General
Product: Mozilla Services → Fennec Native
QA Contact: android-sync → general
Summary: History records being created with GUIDs with trailing newlines → New records being created with GUIDs with trailing newlines
(Assignee)

Comment 3

6 years ago
Created attachment 588442 [details] [diff] [review]
Proposed patch. v1

Just like the current code, this requires v8 or higher.
Attachment #588442 - Flags: review?(blassey.bugs)
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.
Attachment #588442 - Flags: review?(blassey.bugs) → review+
(Assignee)

Comment 5

6 years ago
(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.)
(Assignee)

Comment 6

6 years ago
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!
Blocks: 709339
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
(Assignee)

Updated

6 years ago
Depends on: 662891
(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?
I think we have one working Eclair device in the office. Will check tomorrow.
Richard, please request aurora approval
tracking-fennec: --- → 11+
(Assignee)

Comment 10

6 years ago
Urk, this didn't already make it into Aurora? Oh dear.
(Assignee)

Comment 11

6 years ago
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.
Attachment #588442 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #588442 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/38777175ae77
status-firefox11: --- → fixed
(Assignee)

Updated

6 years ago
Duplicate of this bug: 718762
status-firefox12: --- → fixed
You need to log in before you can comment on or make changes to this bug.