Pinned bookmarks are syncing

RESOLVED FIXED in Firefox 23

Status

()

RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

unspecified
Firefox 23
ARM
Android
Points:
---

Firefox Tracking Flags

(fennec21+)

Details

(Whiteboard: [status-firefox22: fixed])

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

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

Updated

6 years ago
Blocks: 858994
(Assignee)

Comment 2

6 years ago
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!
tracking-fennec: --- → ?
Assignee: nobody → wjohnston
tracking-fennec: ? → 21+

Comment 3

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

Comment 5

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

Updated

6 years ago
Assignee: wjohnston → rnewman
Status: NEW → ASSIGNED
Component: Data Providers → Android Sync
Product: Firefox for Android → Android Background Services
(Assignee)

Comment 6

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

Updated

6 years ago
No longer blocks: 858994
(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+.
(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!
https://hg.mozilla.org/mozilla-central/rev/5f5bbd697d25
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Duplicate of this bug: 877624
(Assignee)

Comment 13

6 years ago
Created attachment 756265 [details] [diff] [review]
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.
Attachment #756265 - Flags: review+
Attachment #756265 - Flags: approval-mozilla-beta?

Updated

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

Comment 14

6 years ago
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.
Whiteboard: [status-firefox22: fixed]

Updated

5 years ago
Duplicate of this bug: 881428

Updated

a year ago
Product: Android Background Services → Firefox for Android
You need to log in before you can comment on or make changes to this bug.