871332 - Bookmarks not syncing between two Firefox Metro paired devices

Status: RESOLVED WONTFIX

(Firefox for Metro Graveyard :: Sync, defect)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: Syncing Log from machine with Bookmarks

Syncing "Bookmarks" is not working between two different Firefox Metro browsers that have been paired. Created 28 bookmarks on the first machine and synced more then a dozen times with the "Bookmarks" not appearing on the second paired Firefox Metro machine.

I've attached a log that hopefully has some more information on what's going on, used the following to retrieve the logs:
services.sync.log.appender.file.logOnSuccess = True
services.sync.log.logger.engine.bookmarks = Trace

Steps to reproduce the issue:

1) On Computer A, setup a "Sync" account going through the "Options" using Firefox Desktop
2) Once you have a "Sync" account created, open Firefox Metro and create several different bookmarks by selecting the "Star" under the "App Bar"
3) Select "Settings" -> "Sync" and then "Pair a device" on Computer A
4) Select "Settings" -> "Sync" and then "Pair a device" on Computer B and pair the two machines
5) "Sync" a few times and you will notice that the "Bookmarks" from Computer A are not being populated on Computer B

Current Behavior:

- Bookmarks from Firefox Metro instances are not being synced to other machines also using Firefox Metro

Expected Behavior"

- Bookmarks should "Sync" between two Firefox Metro's without any issues

Comment 1

[Richard Newman [:rnewman]]

Comment on attachment 748606 [details]
Syncing Log from machine with Bookmarks

Doesn't Metro use a different root for its Metro bookmarks? If so, those probably won't sync. Not in front of a keyboard to check.

Comment 2

[Jim Mathies [:jimm]]

(In reply to Richard Newman [:rnewman] from comment #1)
> Comment on attachment 748606 [details]
> Syncing Log from machine with Bookmarks
> 
> Doesn't Metro use a different root for its Metro bookmarks? If so, those
> probably won't sync. Not in front of a keyboard to check.

I think Kamil was syncing two versions of Metro Firefox, in which case Bookmarks should be mirrored between the two.

Comment 3

[Jim Mathies [:jimm]]

Ally is telling me that the metro root might not sync at all, or if it does, it won't import into the metro root on other machines. If there's an easy fix to this I think we want it. Bookmarks under the Start Screen should be mirrored across two metrofx clients.

Comment 4

[Allison Naaktgeboren :ally]

1368400235597	Sync.Engine.Bookmarks	INFO	0 outgoing items pre-reconciliation
1368400235598	Sync.Engine.Bookmarks	TRACE	Downloading & applying server changes
1368400235598	Sync.Engine.Bookmarks	INFO	Records: 0 applied, 0 successfully, 0 failed to apply, 0 newly failed to apply, 0 reconciled.

Kamil, what of the two machines is this log from?

Comment 5

[Allison Naaktgeboren :ally]

(In reply to Richard Newman [:rnewman] from comment #1)
> Comment on attachment 748606 [details]
> Syncing Log from machine with Bookmarks
> 
> Doesn't Metro use a different root for its Metro bookmarks? If so, those
> probably won't sync. Not in front of a keyboard to check.

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bookmarks.js#12

Comment 6

[Kamil Jozwiak [:kjozwiak]]

(In reply to Jim Mathies [:jimm] from comment #2)
> (In reply to Richard Newman [:rnewman] from comment #1)
> > Comment on attachment 748606 [details]
> > Syncing Log from machine with Bookmarks
> > 
> > Doesn't Metro use a different root for its Metro bookmarks? If so, those
> > probably won't sync. Not in front of a keyboard to check.
> 
> I think Kamil was syncing two versions of Metro Firefox, in which case
> Bookmarks should be mirrored between the two.

Yup, I was syncing between two Firefox Metro browsers on two different machines. Created several Bookmarks on the first Firefox Metro instance and then attempted to mirror it to the second machine also running Firefox Metro (paired with the first machine)

(In reply to :Ally Naaktgeboren from comment #4)
> 1368400235597	Sync.Engine.Bookmarks	INFO	0 outgoing items pre-reconciliation
> 1368400235598	Sync.Engine.Bookmarks	TRACE	Downloading & applying server
> changes
> 1368400235598	Sync.Engine.Bookmarks	INFO	Records: 0 applied, 0 successfully,
> 0 failed to apply, 0 newly failed to apply, 0 reconciled.
> 
> Kamil, what of the two machines is this log from?

The log that I attached is from the machine with all the created Bookmarks. Once I created the Bookmarks, I paired a second Firefox Metro machine and tried mirroring them over. Synced about 5 different times with everything but Bookmarks syncing over.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Jim Mathies [:jimm] from comment #2)

> > Doesn't Metro use a different root for its Metro bookmarks? If so, those
> > probably won't sync. Not in front of a keyboard to check.
> 
> I think Kamil was syncing two versions of Metro Firefox, in which case
> Bookmarks should be mirrored between the two.

Sync only syncs the roots it knows about (kSpecialIds, ["menu", "places", "tags", "toolbar", "unfiled", "mobile"]), so this isn't that it won't add the Metro bookmarks somewhere on desktop, it's that it doesn't see them at all.

Changing this behavior is as simple as making Sync aware of the new root, with the critically important caveat that you should ensure that you don't break other clients (Android, in particular).

"Not break" could even be a good thing: you could add a "Metro bookmarks" top-level alongside "Desktop bookmarks" in the Android bookmark view.

I encourage you to figure this out sooner rather than later, because we you need to fix Android Sync to allow it to pass a new root unmolested, it needs to go out a release or two before this bug.

Comment 8

[Jim Mathies [:jimm]]

rnewman - curious if you know why we create a folder for mobile here? 

http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/bookmarks.js#146

In metro we just use an annotated root node, without a folder - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/bookmarks.js#11

Comment 9

[Matt Brubeck (:mbrubeck)]

(In reply to Jim Mathies [:jimm] from comment #8)
> In metro we just use an annotated root node, without a folder - 
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> bookmarks.js#11

That root node *is* a folder, created as one of our default bookmarks:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/locales/generic/profile/bookmarks.json.in#2

Comment 10

[Richard Newman [:rnewman]]

(In reply to Jim Mathies [:jimm] from comment #8)
> rnewman - curious if you know why we create a folder for mobile here? 
> 
> http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/
> bookmarks.js#146

This code was shared with XUL Fennec, and also shipped as an add-on. In some respects it's defensive for those reasons.

Comment 11

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #9)
> (In reply to Jim Mathies [:jimm] from comment #8)
> > In metro we just use an annotated root node, without a folder - 
> > 
> > http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/
> > bookmarks.js#11
> 
> That root node *is* a folder, created as one of our default bookmarks:
> http://mxr.mozilla.org/mozilla-central/source/browser/metro/locales/generic/
> profile/bookmarks.json.in#2

Ok I see. We have a root folder we create along with some other defaults:

@bookmarks_title@ == 'Bookmarks', item id 6

that has a single annotation on it: 'metro/bookmarksRoot'.

We don't create an entry in moz_bookmarks_roots, not sure what impact that has on all of this.

Comment 12

[Richard Newman [:rnewman]]

For context: Bug 753789.

Comment 13

[Jim Mathies [:jimm]]

Attachment: add support for metro bookmarks v.1

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 757373 [details] [diff] [review]
add support for metro bookmarks v.1

I'll file follow ups on exposing this and setting a proper title on other platforms. I need to run this past Asa first through to be sure we want to do it.

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 757373 [details] [diff] [review]
add support for metro bookmarks v.1

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

This looks good, but I think you need a human-readable title for the root, and I'd like to see xpcshell tests to verify that a device without the root (i.e., non-Metro desktop) can dynamically add a Metro bookmark and have the root create itself on the fly.

It's not clear to me how the METROROOT_FOLDER guid interoperates with the record you create from bookmarks.json.in, which does not have that GUID, but does have the right anno.

::: browser/metro/locales/generic/profile/bookmarks.json.in
@@ +1,3 @@
>  #filter substitution
>  {"type":"text/x-moz-place-container","root":"placesRoot","children":
> +  [{"type":"text/x-moz-place-container","title":"7CCEC092-ED5F-46EF-BBE5-B69F58086271","annos":[{"name":"metro/bookmarksRoot","expires":4,"type":1,"value":1}],

Sounds like you need Bug 627487. You should not be titling the folder with its GUID; that title will sync to other devices and eventually show up on your phone.

Comment 16

[Jim Mathies [:jimm]]

(In reply to Richard Newman [:rnewman] from comment #15)
> Comment on attachment 757373 [details] [diff] [review]
> add support for metro bookmarks v.1
> 
> Review of attachment 757373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but I think you need a human-readable title for the root,

We don't display folder titles, and if we do, can special case this. What I wanted to do is avoid folder name collisions on other devices, and afaict, title, is a actually folder name. Is that not correct? When this folder or title synced to desktop, it wasn't displayed in show all bookmarks. 

> and I'd like to see xpcshell tests to verify that a device without the root
> (i.e., non-Metro desktop) can dynamically add a Metro bookmark and have the
> root create itself on the fly.

Ok, I'll work something up.

> It's not clear to me how the METROROOT_FOLDER guid interoperates with the
> record you create from bookmarks.json.in, which does not have that GUID, but
> does have the right anno.

I guess I'm mixing title and guid up here but the two seemed to be the same from working with the bookmarks engine code. 

> 
> ::: browser/metro/locales/generic/profile/bookmarks.json.in
> @@ +1,3 @@
> >  #filter substitution
> >  {"type":"text/x-moz-place-container","root":"placesRoot","children":
> > +  [{"type":"text/x-moz-place-container","title":"7CCEC092-ED5F-46EF-BBE5-B69F58086271","annos":[{"name":"metro/bookmarksRoot","expires":4,"type":1,"value":1}],
> 
> Sounds like you need Bug 627487. You should not be titling the folder with
> its GUID; that title will sync to other devices and eventually show up on
> your phone.

It didn't show up on desktop. The bookmarks did show up in unsorted though, which is what I wanted.

Comment 17

[Richard Newman [:rnewman]]

(In reply to Jim Mathies [:jimm] from comment #16)

> We don't display folder titles, and if we do, can special case this. What I
> wanted to do is avoid folder name collisions on other devices, and afaict,
> title, is a actually folder name. Is that not correct? When this folder or
> title synced to desktop, it wasn't displayed in show all bookmarks. 

It would display on Android, and it could display on desktop depending on how you wired it in. It'll also show up as the parent name for all synced sub-items, which is unnecessary expense.

> I guess I'm mixing title and guid up here but the two seemed to be the same
> from working with the bookmarks engine code. 

They're not. Title is only used for reconciling. GUIDs and numeric IDs are used to identify items.

The whole ANNO nonsense is used to locate a record whose GUID isn't known in advance (which the built-in Places roots are).

My recommendation is to either:

(a) use the anno
(b) add a places migration and create a real magic-guid root for Metro.

You don't have to support shipping as an add-on, so you can get your hands dirty with Places…

> It didn't show up on desktop. The bookmarks did show up in unsorted though,
> which is what I wanted.

That means desktop just destroyed your data. If a bookmark appears somewhere, that's where it just got *moved* to, because it couldn't find the parent folder. Next time you get node reassigned and desktop syncs first, or you add a new unsorted bookmark on desktop, all of your Metro bookmarks are going to go away (into Unsorted Bookmarks) next time Metro syncs.

Android will probably do the same thing, but even more aggressively, because the reparenting itself will result in an upload.

Note that you still run a risk whenever you add a new root: if you sync your new Metro device with an old Sync client, that client might screw up your stuff. Not fun.