Closed
Bug 1313026
Opened 8 years ago
Closed 7 years ago
Bookmark Sync: Failed to apply incoming record due to malformed guid like SBROWSER_SYNCXXX
Categories
(Firefox :: Sync, defect, P3)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: bugzilla, Unassigned, NeedInfo)
References
Details
Attachments
(1 file)
(deleted),
text/x-log
|
Details |
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.
Component: Untriaged → Sync
OS: Unspecified → All
Hardware: Unspecified → x86
Comment 1•8 years ago
|
||
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Bookmark Sync: Failed to apply incoming record → Bookmark Sync: Failed to apply incoming record due to malformed guid like SBROWSER_SYNCXXX
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•8 years ago
|
||
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 :/
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.
Confirmed, ESR 45.1.0 has no Problems and does the Sync of this Bookmarks.
Comment 6•8 years ago
|
||
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.
Blocks: 1019226
Comment 7•8 years ago
|
||
(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•8 years ago
|
||
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
Hardware: x86 → All
Comment 9•8 years ago
|
||
(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•8 years ago
|
||
(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.
Flags: needinfo?(adavis)
Reporter | ||
Comment 11•8 years ago
|
||
Before i raised this ticket, i have send a Mail to browser@samsung.com. 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 13•8 years ago
|
||
Thanks Richard :)
Comment 14•8 years ago
|
||
Needinfo some Samsung people who may be able to offer insights.
Flags: needinfo?(v.aggarwal)
Flags: needinfo?(praveen.c)
Priority: -- → P3
Comment 15•8 years ago
|
||
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.
Flags: needinfo?(rnewman)
Comment 16•8 years ago
|
||
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.
Flags: needinfo?(rnewman)
Updated•8 years ago
|
Flags: needinfo?(adavis)
Reporter | ||
Comment 17•8 years ago
|
||
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•7 years ago
|
||
No word from Samsung. Please reopen if you'd like to provide information.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(adavis)
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•