858992 - Pinned bookmarks are syncing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, defect)

Description

[Richard Newman [:rnewman]]

And Sync doesn't like them.

This will cause Sync to report partial failures. I need to look at the raw record, see if it's really bad.

1365288914929	Sync.Engine.Bookmarks	DEBUG	J1lu-t8kKGHY mapped to undefined
1365288914929	Sync.Store.Bookmarks	DEBUG	Applying record J1lu-t8kKGHY
1365288914929	Sync.Store.Bookmarks	DEBUG	Local parent is pinned
1365288914929	Sync.Store.Bookmarks	DEBUG	Parent is undefined; reparenting to unfiled.
1365288914930	Sync.Store.Bookmarks	WARN	Failed to apply incoming record J1lu-t8kKGHY
1365288914930	Sync.Store.Bookmarks	WARN	Encountered exception: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.insertBookmark] Stack trace: BStore_create()@resource://gre/modules/services-sync/engines/bookmarks.js:660 < Store_applyIncoming()@resource://services-sync/engines.js:267 < BStore_applyIncoming()@resource://gre/modules/services-sync/engines/bookmarks.js:548 < applyIncomingBatch()@resource://services-sync/engines.js:235 < doApplyBatch()@resource://services-sync/engines.js:814 < resource://services-sync/engines.js:925 < resource://services-sync/record.js:625 < Channel_onDataAvail()@resource://gre/modules/services-sync/resource.js:542 < <file:unknown>
1365288914931	Sync.Engine.Bookmarks	DEBUG	P1vJltCDAHWz mapped to undefined

Comment 1

[Richard Newman [:rnewman]]

Ah, this'll do it.

[16:09:40.361] ({tags:[], id:"J1lu-t8kKGHY", parentid:"pinned", title:"weather 94127", type:"bookmark", bmkUri:"weather 94127", parentName:"Pinned"})


So, there are two bugs here:

* Fennec and/or Sync doesn't ignore children of the pinned bookmarks folder when uploading bookmarks.

* Fennec allows you to create a bookmark whose URI is not a URI, at least as a pinned item.

I'll file a second bug for that.

Comment 2

[Richard Newman [:rnewman]]

Tracking because a completed sync will result in your pinned bookmarks moving into Unsorted Bookmarks, and thus becoming unpinned. This only doesn't happen now because of Bug 858994 causing the sync to fail!

Comment 3

[:Margaret Leibovic]

What's happening with this bug? Is this something that should have been fixed by bug 826539? It seems like this might belong in the Android Sync component.

I'm working on addressing the bad URI issue in bug 858994, but if there's a way for us to just stop syncing pinned bookmarks altogether, that would solve the more pressing issue of sync errors appearing on desktop.

Comment 4

[Nick Alexander :nalexander [he/him]]

Sorry, haven't gotten to looking at Android Sync side of this yet.  rnewman and I are aware of it and will get to it next week.

Comment 5

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #3)
> What's happening with this bug? Is this something that should have been
> fixed by bug 826539? It seems like this might belong in the Android Sync
> component.

I didn't move it (yet), because the code in that bug looks like it should work, and so there might be some other odd thing happening here, like Fennec not including the correct GUID in the returned cursor rows. When we figure out what's going wrong, we'll know where to put the fix :D
 
> I'm working on addressing the bad URI issue in bug 858994, but if there's a
> way for us to just stop syncing pinned bookmarks altogether, that would
> solve the more pressing issue of sync errors appearing on desktop.

We need to fix this regardless, yes.

(In reply to Nick Alexander :nalexander from comment #4)
> Sorry, haven't gotten to looking at Android Sync side of this yet.  rnewman
> and I are aware of it and will get to it next week.

If I'm lucky, I'll get to it in the next fifteen minutes... *crosses fingers*

Comment 6

[Richard Newman [:rnewman]]

Update on this: I was either sloppy or too smart for my own good in Bug 826539. That bug prevents incoming pinned items from being applied to an Android device. That neglected any effect on desktop; were it not for those side-effects, it would be the simplest solution, and closest to having syncable pinned sites!

I have a patch in the works that will expand the set of non-uploaded records to match.

Comment 7

[Richard Newman [:rnewman]]

Fix and test:

https://github.com/mozilla-services/android-sync/pull/309

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #7)
> Fix and test:
> 
> https://github.com/mozilla-services/android-sync/pull/309

Review comments are up on github -- code=r+, knowledge transfer=f+.

Comment 9

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5bbd697d25

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #9)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5bbd697d25

knowledge-transfer++ -- updated comments look great.  Thanks!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f5bbd697d25

Comment 13

[Richard Newman [:rnewman]]

Attachment: Patch for uplift.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Addition of pinned sites without adequate protection being added to Sync. Bug 826539 wasn't enough.

User impact if declined: 
  Pinned sites in Fennec will be lost (see Bug 877624 for an example user report).

Testing completed (on m-c, etc.): 
  Baked on m-c and Aurora.

Risk to taking this patch (and alternatives if risky): 
  Slim, and if something does go wrong, it'll be limited to Sync users. No alternatives.

String or IDL/UUID changes made by this patch:
  None.

Comment 14

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-beta/rev/67b0e4327b11

Tracking flags seem to be... missing in this component. Humph. Alex, if you'd prefer, feel free to set the Target Milestone to 22.