1313026 - Bookmark Sync: Failed to apply incoming record due to malformed guid like SBROWSER_SYNCXXX

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect, P3)

Description

[Theraten]

Attachment: sync.log

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160922113459

Steps to reproduce:

Add a Bookmark on another Bowser or Android Device and Sync that newly added Bookmarks to Firefox Desktop


Actual results:

The newly added entrys does not show up on Firefox Desktop, they are only added in Firefox Mobile on Android.


Expected results:

The newly added entrys should be synced to all devices.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Something, probably an addon or similar, has created a bookmark with a GUID of SBROWSER_SYNC1477379050528, which I'm guessing is the problem here.  Which got me thinking - PlacesUtils and PlacesSyncUtils have their own local validators for input args creating bookmarks - ISTM we could probably wrangle the bookmark validator to apply a remote record against the local validator via existing PlacesSyncUtils methods (possibly with minor changes so validation can be done without actually creating) to see if they could actually be applied?

Comment 2

[Theraten]

The Bookmark comes from the "Samsung Browser" on a Android Phone (https://play.google.com/store/apps/details?id=com.sec.android.app.sbrowser), which uses the Firefox Sync also. The Sync works before, since a long time, without a Problem. This let me think, something has changed on Mozilla Side.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Oh right, I wasn't aware that has FxA sync support. This is Firefox 49, right? I can't recall changes in that version which would explain this - but can predict challenges in 50 :/

Comment 4

[Theraten]

Yes its on Firefox 49.0.2. I have tested Nightly from 25.10 with the same result. I didn't made a Test with latest ESR at the Moment but i could try it.

Comment 5

[Theraten]

Confirmed, ESR 45.1.0 has no Problems and does the Sync of this Bookmarks.

Comment 6

[Lina Butler [:lina]]

I think this is bug 1019226. ESR 45 doesn't pass a GUID (https://dxr.mozilla.org/mozilla-esr45/rev/7006b275b8290cbf02b57d1f98e9d33857ccb9f3/services/sync/modules/engines/bookmarks.js#746-747,776-777,834-835) and relies on de-duping; Firefox 48+ uses the server's GUID.

We can check if the GUID is valid, and generate a new one if it's not. We could also ignore bookmarks without a valid GUID, but that's a really bad experience for Samsung Browser users.

Comment 7

[Lina Butler [:lina]]

(In reply to Mark Hammond [:markh] from comment #1)
> ISTM we could probably
> wrangle the bookmark validator to apply a remote record against the local
> validator via existing PlacesSyncUtils methods (possibly with minor changes
> so validation can be done without actually creating) to see if they could
> actually be applied?

That's a good idea. We could at least expose `validateSyncBookmarkObject`, and maybe do fancier checks like seeing if the record would be an orphan, or would have a different kind when applied.

Comment 8

[Richard Newman [:rnewman]]

If only SBrowser obeyed the spec…

---
BSO ids must only contain printable ASCII characters. They should be exactly 12 base64-urlsafe characters; while this isn’t enforced by the server, the Firefox client expects it in most cases.
---
http://docs.services.mozilla.com/storage/apis-1.5.html

Comment 9

[Mark Hammond [:markh] [:mhammond]]

(In reply to Kit Cambridge [:kitcambridge] from comment #6)
> We can check if the GUID is valid, and generate a new one if it's not.

IIUC this would effectively end up as a delete of the remote record and the creation of a new identical one but with a valid GUID? ISTM we really need a samsung device (or emulator) to test this on - best I can tell, the browser can't be loaded on non Samsung devices.


(In reply to Richard Newman [:rnewman] from comment #8)
> If only SBrowser obeyed the spec…

Does anyone know how we could report this bug to them?

Comment 10

[Mark Hammond [:markh] [:mhammond]]

(In reply to Mark Hammond [:markh] from comment #9)
> (In reply to Richard Newman [:rnewman] from comment #8)
> > If only SBrowser obeyed the spec…
> 
> Does anyone know how we could report this bug to them?

Alex, are you able to help us here? The short story is that Samsung browsers are syncing in a way that doesn't conform to the spec, and our recent changes mean this violation is suddenly important. While we might be able to help not totally break Samsung users, I'd be reluctant to do so without a commitment they will change the browser to conform to the spec - we don't want them writing invalid records indefinitely.

Comment 11

[Theraten]

Before i raised this ticket, i have send a Mail to [e-mail]. According to Google play, this is the Address where i could report things to the developers. Afterwards, i have added the Link to this Bug to the Mail conversation. No response as of yet.

if you need more test, i could do on my side, feel free to give me some hints, what you need. I use a Samsung Galaxy S6 with Samsung Browser installed. In my work time im a ISTQB certified Softwaretester. 

Maybe another way would it, to install the APK on a Emulator. You could find APK's at http://www.apkmirror.com/apk/samsung-electronics-co-ltd/samsung-internet-for-android/ The most resent Version on that side must not be equal to the latest official Version on the Playstore.

Comment 12

[Richard Newman [:rnewman]]

Yeah, I know some folks. CCed.

Comment 13

[Alex Davis [:adavis]]

Thanks Richard :)

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Needinfo some Samsung people who may be able to offer insights.

Comment 15

[Lina Butler [:lina]]

Richard, would you mind reaching out to the folks at Samsung? Alternatively, we can look at removing GUID validation. It already causes problems for items like "readinglist", and we don't seem to gain much from it apart from tests.

Comment 16

[Richard Newman [:rnewman]]

Done.

We pushed quite hard to get a consistent, validatable, compact GUID representation for Sync 1.5, and every other client meets the spec. I'm not inclined to give that up.

I'll also note that SBROWSER_SYNC1477379050528 looks like it's based on a timestamp, which is generally a bad idea for a syncing system; there might be a bug lurking under this bad decision.

Comment 17

[Theraten]

Are there any Info from Samsung? Meanwhile i have switched completely to Firefox also on Mobile. Only thing which i hate on Mobile Firefox is that URL's to other Apps are not opened directly in that App and you have to click the small Android Button in addressbar.

Comment 18

[Julie McCracken (:julie)]

No word from Samsung. Please reopen if you'd like to provide information.