Syncing bookmarks between desktop and current mobile build (14) causes bookmarked feeds to turn into empty folders.

VERIFIED FIXED in Firefox 15

Status

()

Firefox for Android
General
P1
critical
VERIFIED FIXED
5 years ago
10 months ago

People

(Reporter: jared.marc, Assigned: gcp)

Tracking

({dataloss})

unspecified
Firefox 17
ARM
Android
dataloss
Points:
---

Firefox Tracking Flags

(firefox14 wontfix, firefox15+ verified, firefox16+ verified, firefox17 verified, blocking-fennec1.0 .N+)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.0; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120614114901

Steps to reproduce:

Synched bookmarks on mobile.


Actual results:

RSS/Atom feeds on desktop were turned into empty regular folders after syncing.


Expected results:

Feeds on desktop should have stayed as feeds.
(Reporter)

Updated

5 years ago
OS: Windows Vista → Android

Updated

5 years ago
Component: General → Android Sync
Product: Fennec Native → Mozilla Services
QA Contact: general → android-sync
Version: Firefox 14 → unspecified
Thanks for the report.

We should be round-tripping livemarks, and we have some test coverage for this.

Do you have a log from desktop and/or mobile reproducing this?

http://160.twinql.com/how-to-file-a-good-android-sync-bug
Keywords: qawanted
Hardware: x86 → ARM
(Reporter)

Comment 2

5 years ago
(In reply to Richard Newman [:rnewman] from comment #1)
> Thanks for the report.
> 
> We should be round-tripping livemarks, and we have some test coverage for
> this.
> 
> Do you have a log from desktop and/or mobile reproducing this?
> 
> http://160.twinql.com/how-to-file-a-good-android-sync-bug

I do not. I currently have the sync setting on my phone for firefox sync disabled, so I don't have to go through the process on my desktop of restoring my bookmarks to a previous time, since firefox freezes up for as much as 20 minutes restoring all the many bookmarks I have.  I'd get the log if I could if it wasn't such a pain to go through.
I am unable to reproduce the issue on Nightly 16.0a1 2012-07-02 on HTC Desire running Android 2.2.2 or Firefox Mobile 14(Google play official version) on Motorola Droid Pro running Android 2.3.4. I have tried a few feeds from http://www.intertwingly.net/wiki/pie/KnownAtomFeeds - BlogZiNet, telligent - , dropbox feeds and cbs feeds. 

Can you please provide a few websites/feed locations to try and reproduce this issue.
Duplicate of this bug: 770394
QA: please take a look at this. Some examples, including screenshots, in <https://support.mozilla.org/en-US/questions/930847>
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
Created attachment 638635 [details]
sync logs

I was able to reproduce the issue but only after doing a migration from firefox 10 to 14.

Steps to reproduce:
1) Add feeds as bookmarks ( use http://feeds2.feedburner.com/techradar/allnews and http://www.theregister.co.uk/headlines.atom)
2) Install Firefox Mobile 10 from the ftp server.
3) Setup sync and wait for the sync process to finish.
4) From Google Play update Firefox Mobile to 14.
5) Wait for migration to finish.
6) After 10 minutes do a sync on desktop.

Results:
The feeds are transformed into empty folders.
After migration finishes a sync process is started and at some point in the process the feed bookmarks are modified. This sync process then takes 10-15 minutes to complete. Logs indicate that number of visits are added to the history items.

I will try to see again if I can reproduce this without migrating the profile.
Created attachment 638636 [details]
desktop web console errors
Keywords: qawanted
Created attachment 638654 [details]
Google Nexus sync logs

The issue was reproduced on Motorola Droid Pro (Android 2.3.4) and Samsung Google Nexus (Android 4.0.2) but only using the steps provided in comment 6. I was unable to reproduce the issue with a fresh install of Firefox Mobile from the Google Play store.
Perhaps profile migration is not preserving live marks correctly? gcp, any insight?
tracking-fennec: --- → ?
blocking-fennec1.0: --- → ?
Keywords: dataloss
(Assignee)

Comment 10

5 years ago
Livemarks are contained in folders. These are represented as folders in Places, and are migrated as folders in Native Fennec.

The actual Livemark information is in auxiliary Places tables (moz_anno_attributes, moz_annos, moz_items_annos). They aren't supported by Native Fennec and aren't migrated.
blocking-fennec1.0: ? → .N+
Yeah, that isn't enough: you need to detect special places folder types and translate the schema appropriately during migration. The right thing probably happens already for separators, but it would be good to check queries etc.
Component: Android Sync → General
Product: Mozilla Services → Fennec Native
QA Contact: android-sync → general
(Assignee)

Comment 12

5 years ago
I don't understand what you mean. As I already said, Livemarks aren't supported in Native Fennec, and hence, there is nothing that can be migrated.
(Assignee)

Comment 13

5 years ago
From what I understand in this bug, Sync is looking at the bookmarks in Native Fennec, seeing that it has some folders (the containing ones that *can* be migrated) that are empty (and I guess have modification times at or newer than desktop) and then comparing to the ones in Desktop.

Then it makes the conclusion that because the ones in Native Fennec don't have Livemarks in them (which, again, *are not supported* in Native Fennec), and the ones in Desktop do, that the Livemarks in desktop should be deleted.

The bug is that absence of a feature/tag/mark in a Native Fennec bookmark/history/etc item should *never* cause Sync to delete this on other platforms if the feature *doesn't exist on Native Fennec in the first place*.

This can never be fixed by Migration for reasons already mentioned, furthermore the (apparently) broken logic above will still cause dataloss for all current users no matter what we do wrt to Livemarks and Migration (which will never even run again as 1.0 is out!) even if we'd add Livemarks to Native Fennec at some point.

Just in case that is not clear, let me repeat: the containing Folders look like and are marked as normal folders in Places and normal Native Fennec. Migration will translate them as normal folders. (*Not* doing so would be dataloss too!) The flags identifying them as Livemarks exist entirely outside of the bookmarks/places/history table *and are not supported in Native Fennec at all*.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #12)
> I don't understand what you mean. As I already said, Livemarks aren't
> supported in Native Fennec, and hence, there is nothing that can be migrated.

Well, that explains the confusion. Yes they are:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/BrowserContract.java.in#92


> The bug is that absence of a feature/tag/mark in a Native Fennec
> bookmark/history/etc item should *never* cause Sync to delete this on other
> platforms if the feature *doesn't exist on Native Fennec in the first place*.

It's not the absence of an attribute; it's a totally different type. And it's not deletion, it's record replacement. We'd do the same thing if someone killed the livemark attribute of a desktop livemark; it'd be propagated as a folder to other devices.

We put a fair amount of effort into making sure that Fennec's schema supports everything that Sync wants to store (even if Fennec won't display it), and that generally means it has a large overlap with Places.

This is why we fixed Bug 708149 back in March. Fennec supports round-tripping all of the desktop data that Sync delivers, including livemarks, separators, and queries. Sync will add livemarks and such to Fennec's DB; there's no reason why profile migration can't do the same when it encounters a livemark in places.sqlite.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #13)
> From what I understand in this bug, Sync is looking at the bookmarks in
> Native Fennec, seeing that it has some folders (the containing ones that
> *can* be migrated) that are empty (and I guess have modification times at or
> newer than desktop) and then comparing to the ones in Desktop.

It's the 'folder' that's the livemark (or "livemark container"); livemarks contain "livemark items".

"Livemarks are bookmark folders that contain the most recent items from an RSS feed."
(https://developer.mozilla.org/en/Using_the_Places_livemark_service)

On a first sync after migration, Sync will download records, see a livemark, compare it to the local migrated record with the same GUID, see that the local record is changed and newer, and subsequently upload the winning record.

Other clients will then see the changed record on their next sync, and apply it to their local database. In the case of our clients, that will discard the local livemark and replace it with the folder.

> Just in case that is not clear, let me repeat: the containing Folders look
> like and are marked as normal folders in Places and normal Native Fennec.

That's not strictly true; livemarkService.isLivemark() returns true for livemarks and false for folders.

<https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsILivemarkService#isLivemark%28%29>
(Assignee)

Comment 16

5 years ago
>Well, that explains the confusion. Yes they are:

Yikes. I'll be proactive and ask: what are queries?

>That's not strictly true; livemarkService.isLivemark() returns true for livemarks 
>and false for folders.

That might be true, but it says nothing about how it's implemented. Margaret added a check so Profile Migration only migrates things it knows about, see bug 754276. If Livemarks are being migrated, it's because they're pretending to be folders in Places (and that's what I think I saw when investigating).

The problem is that adding support for Livemarks to Profile Migration (i.e. fixing it to "known" they aren't really folders and skip them or import them with that type set) isn't going to solve anything because Profile Migration will never run again.
(Assignee)

Comment 17

5 years ago
Is it possible for to Sync to know that "Folder" in Native Fennec might also mean "Livemark" and that it shouldn't do replacement if it encounters identical records that only differ there?
No, alas. This is a published protocol, with deployed clients going back to Firefox 4. Any fix would have to be a second migration step. (And correcting migration for laggards, of course.)
(Assignee)

Comment 19

5 years ago
I don't mean uploading the broken information and hacking this in all clients - I mean recognizing this and fixing it locally in the Android client. Can the Android client fetch first and see if there's already a livemark with the exact same GUID for a folder it has stored, and infer the folder is a livemark, before sending out its own information?

i.e. you said:

>On a first sync after migration, Sync will download records, see a livemark, 
>compare it to the local migrated record with the same GUID, see that the local 
>record is changed and newer, and subsequently upload the winning record.

You could change the scoring algorithm here (Android-only) to let the right record win. 

The downside would be that if the user creates a folder with the exact same name and GUID as a bookmark folder then it won't be Synced, but this is likely impossible?

I don't see much other options. Migration will have deleted the data that would have been needed to identify the folder as a livemark. The only other thing I can think of is to make a hack so empty bookmark folders that haven't been modified since migration ran (can we even see when that was?) are deleted or at the very least marked so they won't be picked up by Sync. 
"What could possibly go wrong?"
(In reply to Gian-Carlo Pascutto (:gcp) from comment #16)
> >Well, that explains the confusion. Yes they are:
> 
> Yikes. I'll be proactive and ask: what are queries?

It's a bookmark whose URI begins "place:"; they're used to implement things like bookmark shortcuts, saved searches, and so on. We store those specially in Fennec, too, because Fennec doesn't know what to do with them, but we need to round-trip for desktop.

> >That's not strictly true; livemarkService.isLivemark() returns true for livemarks 
> >and false for folders.
> 
> That might be true, but it says nothing about how it's implemented.

No, but it indicates that the information is there in the DB.

Sync uses:

    switch (type) {
      case bms.TYPE_FOLDER:
        if (PlacesUtils.annotations
                       .itemHasAnnotation(itemId, PlacesUtils.LMANNO_FEEDURI)) {
          return "livemark";
        }
        return "folder";

That is, it directly looks in the annotations table (via the anno API, but you could use SQL). Adding a couple of joins to the bookmark query would be enough to flag things that look like folders as folders or livemarks during migration.

> added a check so Profile Migration only migrates things it knows about, see
> bug 754276. If Livemarks are being migrated, it's because they're pretending
> to be folders in Places (and that's what I think I saw when investigating).

It's fairer to say "livemarks are a type of folder", rather than "pretend" -- they just happen to be implemented as annotations on folders, rather than extending an enumeration. In Fennec we had the opportunity to redesign the schema, so they're a standalone type.

The root of this bug is that profile migration behaves as if there's a 1:1 correspondence between Places type code and Fennec type code, which is not the case.

> The problem is that adding support for Livemarks to Profile Migration (i.e.
> fixing it to "known" they aren't really folders and skip them or import them
> with that type set) isn't going to solve anything because Profile Migration
> will never run again.

Surely we haven't reached 100% user upgrade penetration, particularly on tablets?

Any fix we might apply in Sync has hardly any more ability to help users -- any that have a Sync account will already have had their livemarks un-livified.

So I suggest we:

* Extend profile migration to understand the full range of Places datatypes, which will avoid this issue entirely for users who haven't yet upgraded, or whose devices aren't yet eligible for Fennec Native, but soon will be.

* Ensure that QA tests profile migration with a non-trivial profile -- this kind of thing should have been caught before release.

* Add a shim to Sync to not downgrade records in this situation, which will avoid this issue for the handful of users who had a XUL Fennec profile with Sync, but did not have their Sync account migrated, and who will set up Android Sync in the future.

* Point affected users to the SUMO docs for restoring desktop bookmarks from the automated backups that we make.


(In reply to Gian-Carlo Pascutto (:gcp) from comment #19)
> I don't mean uploading the broken information and hacking this in all
> clients - I mean recognizing this and fixing it locally in the Android
> client. Can the Android client fetch first and see if there's already a
> livemark with the exact same GUID for a folder it has stored, and infer the
> folder is a livemark, before sending out its own information?

Kinda; migration always runs before a first sync, so this is an extension of the guid-already-exists reconciling logic. I wouldn't object to adding a hack here, but it's by no means a substitute for fixing migration: adding this to Sync would only help users who have set up Sync on XUL, and disabled it before re-enabling Sync after migration. That should be a really small set of users.

Users who subscribed to feeds in XUL Fennec but never used Sync, or users who already migrated with Sync enabled, will still be screwed -- if they had Sync enabled then we've already overwritten the data on the server and on other devices.


> I don't see much other options. Migration will have deleted the data that
> would have been needed to identify the folder as a livemark. The only other
> thing I can think of is to make a hack so empty bookmark folders that
> haven't been modified since migration ran (can we even see when that was?)
> are deleted or at the very least marked so they won't be picked up by Sync. 
> "What could possibly go wrong?"

If the device has already synced (which it will have), then the cat is out of the bag, and these hacks are pointless; the old livemark record doesn't exist anywhere any more. And if they haven't synced, then we might as well just fix migration...
(Assignee)

Comment 21

5 years ago
>It's a bookmark whose URI begins "place:"..We store those specially in Fennec, 
>too, because Fennec doesn't know what to do with them, but we need to round-trip 
>for desktop.

They'll be broken too, though your description gives me some hope they may still be fixable post-factum.

>No, but it indicates that the information is there in the DB.
...
>The root of this bug is that profile migration behaves as if there's a 1:1 
>correspondence between Places type code and Fennec type code, which is not the 
>case.

Right. I was pointing out that because they're marked as folders in Places, the logic that was supposed to not migrate stuff Profile Migration doesn't understand, failed. Furthermore, when bug 708149 landed it included a change to Profile Migration to set the folder type, but the consequence of not supporting the other added types wasn't addressed (probably because no-one realized it).

>Surely we haven't reached 100% user upgrade penetration, particularly on tablets?

I have no idea how many users we left behind, but note that anyone who used Firefox enough to have used XUL *and* Sync is unlikely to be the kind of user that hasn't upgraded by now.

>* Ensure that QA tests profile migration with a non-trivial profile -- this kind of thing 
>should have been caught before release.

Nod - to be honest I'm surprised this wasn't noticed in Beta as it's so severe.

>* Add a shim to Sync to not downgrade records in this situation, which will avoid this issue for 
>the handful of users who had a XUL Fennec profile with Sync, but did not have their Sync account 
>migrated, and who will set up Android Sync in the future.

Hmm, I think you're right here that adding a workaround in Sync will not help anyone. You can only get this problem if you had Sync set up in XUL, and we migrated Sync credentials. It takes an extraordinary set of circumstances for the fix to still be able to save you.

>If the device has already synced (which it will have), then the cat is out of the bag, and these hacks are 
>pointless; the old livemark record doesn't exist anywhere any more. And if they haven't synced, then we 
>might as well just fix migration...

Fixing migration will be too late - it starts on the first run. Unless they've never used Fennec and will update once more before starting it. Not bloody likely.

I think at this point we can only fix this for Tablets and offer our apologies to the other users. And ask for more Beta testers, so this doesn't happen again.
(In reply to Gian-Carlo Pascutto (:gcp) from comment #21)

> Hmm, I think you're right here that adding a workaround in Sync will not
> help anyone. You can only get this problem if you had Sync set up in XUL,
> and we migrated Sync credentials. It takes an extraordinary set of
> circumstances for the fix to still be able to save you.

Unless we allowed the creation of livemarks or queries in XUL Fennec. If we didn't, then non-Synced profiles won't be affected. If we did, then it's still worth guarding, because a user who has never used Sync could end up with data loss.

Do you know if XUL Fennec had that capability?

> I think at this point we can only fix this for Tablets and offer our
> apologies to the other users. And ask for more Beta testers, so this doesn't
> happen again.

No chance that places.sqlite, or the annos in the DB, would still be hanging around?
(In reply to Gian-Carlo Pascutto (:gcp) from comment #21)
> I think at this point we can only fix this for Tablets and offer our
> apologies to the other users. And ask for more Beta testers, so this doesn't
> happen again.

Fewer than 40% of Fennec 10 users have upgraded to Fennec 14 so far (estimating based on blocklist ping metrics), so for the remaining users it is still useful to fix migration in 14.0.1 if possible.  Is there any low-risk change we can make to the profile migrator to avoid triggering this?  What if it just skipped livemarks during migration?
(In reply to Matt Brubeck (:mbrubeck) from comment #23)

> Fewer than 40% of Fennec 10 users have upgraded to Fennec 14 so far
> (estimating based on blocklist ping metrics), so for the remaining users it
> is still useful to fix migration in 14.0.1 if possible.  Is there any
> low-risk change we can make to the profile migrator to avoid triggering
> this?  What if it just skipped livemarks during migration?

The problem is that profile migration can't distinguish between folders and livemarks, so skipping them would be exactly as much work as fixing the bug.

My recommendation is to extend the migration query to join on moz_items_annos, and augment the conditional for setting the Fennec bookmark type-code and URL. This should be low risk, is eminently testable, and should be quite self-contained.

I would be happy to review a patch.
(Assignee)

Comment 25

5 years ago
>No chance that places.sqlite, or the annos in the DB, would still be hanging 
>around?

That's the same thing. For users with a very big history, it might take us several Sync runs to finish and delete it.

I could introduce a new SharedPreference, LIVEMARKS_MIGRATED_DONE or something, change Profile Migration to check for that too, and if not, see if the DB is still there and how much damage is fixable. Then we have to make a query for the livemarks, get their GUIDs from BrowserProvider, and make the adjustments.

This is quickly getting more complicated, though.

>My recommendation is to extend the migration query to join on moz_items_annos, and 
>augment the conditional for setting the Fennec bookmark type-code and URL. This 
>should be low risk, is eminently testable, and should be quite self-contained.

The bookmarks query already joins over 4 tables, I don't want to add 3 more. I think I'll make it a seperate query that just gets the id's of the livemark records in Places, puts them in a Set, and checks that set during bookmarks migration. Should be even more standalone.

I presume queries can be handled similarly.
Assignee: nobody → gpascutto
(In reply to Gian-Carlo Pascutto (:gcp) from comment #25)

> The bookmarks query already joins over 4 tables, I don't want to add 3 more.
> I think I'll make it a seperate query that just gets the id's of the
> livemark records in Places, puts them in a Set, and checks that set during
> bookmarks migration. Should be even more standalone.

Sounds good to me; that's equivalent to an in-memory join.

> I presume queries can be handled similarly.

Even more easily, yes; check for the URI starting with "place:" during processing.

Note that your livemark and query checks should translate the Places attributes (PlacesUtils.LMANNO_FEEDURI (feedUri), PlacesUtils.LMANNO_SITEURI (siteUri), "Places/SmartBookmark" (queryId)) into a "magic" URI for storing in Fennec, which doesn't have those columns:

https://github.com/mozilla-services/android-sync/blob/develop/src/main/java/org/mozilla/gecko/sync/repositories/domain/BookmarkRecord.java#L264

https://github.com/mozilla-services/android-sync/blob/develop/src/test/java/org/mozilla/android/sync/test/TestRecord.java#L219
Blocks: 771214
(Assignee)

Comment 27

5 years ago
Created attachment 640995 [details] [diff] [review]
Patch 1. Read in and apply extra bookmark attributes

I didn't manage to construct any query that has a queryId set (and would be among the imported bookmarks), so that part is untested.
Attachment #640995 - Flags: review?(rnewman)
Comment on attachment 640995 [details] [diff] [review]
Patch 1. Read in and apply extra bookmark attributes

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

::: mobile/android/base/ProfileMigrator.java
@@ +194,5 @@
> +    private static final String ANNO_CONTENT   = "a_content";
> +
> +    private class AttributePair {
> +        String mName;
> +        String mContent;

These should be marked as final. And given that they're immutable, I'm inclined to name these fields "name" and "content", so you can use "pair.content".

(Rule of thumb: if you ever see foo.mBar in code that isn't a method on Foo, you either need an accessor which can enforce concurrency decisions and/or you should make `bar` final. This is a pseudo-struct, so just go for immutability.)

@@ +214,5 @@
>      private static final int PLACES_TYPE_SEPARATOR = 3;
> +    // These aren't used in the type field in places,
> +    // but we use them internally.
> +    private static final int PLACES_TYPE_LIVEMARK  = 4;
> +    private static final int PLACES_TYPE_QUERY     = 5;

Can we add a comment here explaining that these have nothing to do with the type codes in BrowserContract.Bookmarks? The numbers don't match up, which could be confusing as hell for the poor sods who come after us.

@@ +1022,5 @@
>              // Translate the parent pointer if needed
>              if (mRerootMap.containsKey(parent)) {
>                  parent = mRerootMap.get(parent);
>              }
>              // The bookmark can only be one of three valid types

Comment is no longer true.

@@ +1050,5 @@
> +                cursor.moveToFirst();
> +                while (!cursor.isAfterLast()) {
> +                    long id = cursor.getLong(idCol);
> +                    String attName = cursor.getString(nameCol);
> +                    String content = cursor.getString(contentCol);

These can all be final.

@@ +1054,5 @@
> +                    String content = cursor.getString(contentCol);
> +
> +                    if (attName.equals(PLACES_ATTRIB_QUERY)
> +                        || attName.equals(PLACES_ATTRIB_LIVEMARK_FEED)
> +                        || attName.equals(PLACES_ATTRIB_LIVEMARK_SITE)) {

Unless you've got a style guide that wants operators at start of line, put them at end of line, please!

You also want a null check here for attName, or reverse these checks:

  if (PLACES_ATTRIB_QUERY.equals(attName) || …

@@ +1079,5 @@
> +
> +            return attributes;
> +        }
> +
> +        protected int augmentBookmark(Map<Long, List<AttributePair>> attributes,

This could really do with a brief JavaDoc explaining precisely what kind of augmentation occurs. Perhaps you want a name like "discriminateBookmarkTypes"?

I'd also suggest ensuring that `attributes` is `final` as a hint that it's not to be modified.

And finally, can't this method be declared as `static`?

@@ +1094,5 @@
> +            if (!attributes.containsKey(id)) {
> +                return type;
> +            }
> +
> +            List<AttributePair> list = attributes.get(id);

`final`. And I think you need a null check here, because the for-each syntax is not null safe.

@@ +1100,5 @@
> +                if (pair.mName.equals(PLACES_ATTRIB_QUERY)) {
> +                    type = PLACES_TYPE_QUERY;
> +                    if (!TextUtils.isEmpty(pair.mContent)) {
> +                        if (urlBuffer.length() > 0) urlBuffer.append("&");
> +                        urlBuffer.append("queryId=" + pair.mContent);

Where is the '?' going to be added?
Attachment #640995 - Flags: review?(rnewman) → feedback+
(Assignee)

Comment 29

5 years ago
>Where is the '?' going to be added?

Please clarify, I have absolutely no idea what you are talking about.
(Assignee)

Comment 30

5 years ago
Created attachment 642561 [details] [diff] [review]
Patch 1 v2. Read in and apply extra bookmark attributes

Implemented review comments. 

>And finally, can't this method be declared as `static`?

Not possible in an inner class, unless I pull it entirely out of PlacesRunnable, but I don't think that helps readability/organization of the code.

>And I think you need a null check here, because the for-each syntax is not null safe.

The containsKey one line up assures the key exists. If the key it exists it can't be null due to the logic that constructs the hash.
Attachment #640995 - Attachment is obsolete: true
(Assignee)

Comment 31

5 years ago
Comment on attachment 642561 [details] [diff] [review]
Patch 1 v2. Read in and apply extra bookmark attributes

rnewman, if you can't r+ this code, can you suggest someone who can? The Sync interaction is the most important part of this code, so if you are confident it will work (or at least will be better than what we had!), then I don't think anyone else is needed?
Attachment #642561 - Flags: review?(rnewman)
Attachment #642561 - Flags: feedback+
f+ after r? means "looks good, but I want to see the revised patch rather than landing with nits addressed".
Status: NEW → ASSIGNED
Comment on attachment 642561 [details] [diff] [review]
Patch 1 v2. Read in and apply extra bookmark attributes

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

'?' was me misremembering the URI format we used!

Only one thing to fix.

::: mobile/android/base/ProfileMigrator.java
@@ +1118,5 @@
> +                } else if (PLACES_ATTRIB_LIVEMARK_FEED.equals(pair.name)) {
> +                    type = PLACES_TYPE_LIVEMARK;
> +                    if (!TextUtils.isEmpty(pair.content)) {
> +                        if (urlBuffer.length() > 0) urlBuffer.append("&");
> +                        urlBuffer.append("feedUri=" + pair.content);

You need to URI-encode the content in these three spots.
Attachment #642561 - Flags: review?(rnewman) → review+
(Assignee)

Comment 34

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e444abb9f5e6
Target Milestone: --- → Firefox 17
(Assignee)

Comment 35

5 years ago
Comment on attachment 642561 [details] [diff] [review]
Patch 1 v2. Read in and apply extra bookmark attributes

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Profile Migration
User impact if declined: Loss of livemarks and Places queries for users migration from XUL
Testing completed (on m-c, etc.): Just landed on m-c
Risk to taking this patch (and alternatives if risky): Potential broken migration (more than right now :-)
Attachment #642561 - Flags: approval-mozilla-beta?
Attachment #642561 - Flags: approval-mozilla-aurora?
status-firefox14: --- → affected
status-firefox15: --- → affected
status-firefox16: --- → affected
https://hg.mozilla.org/mozilla-central/rev/e444abb9f5e6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: qawanted
This isn't going to make our first beta due to timing and risk mitigation. Brad let me know that if this doesn't make it into our first beta, we'll get significantly less testing.

Given that, we'll want to QA this in a very focused manner and watch for migration feedback on nightly/aurora.

Updated

5 years ago
tracking-firefox15: --- → +
tracking-firefox16: --- → +

Updated

5 years ago
Attachment #642561 - Flags: approval-mozilla-beta?
Attachment #642561 - Flags: approval-mozilla-beta+
Attachment #642561 - Flags: approval-mozilla-aurora?
Attachment #642561 - Flags: approval-mozilla-aurora+
status-firefox14: affected → wontfix
Unable to reproduce the issue following the steps from Comment 6 on Nighlty 17.0a1 2012-07-19/Moghtly XUL 17.0a1 2012-07-19 using an HTC Desire running Android 2.2.

Feeds are no longer synced back as empty folders, after migration they do not appear on the device and still work as expected on Desktop. Also after migration new bookmarks from both Desktop and Mobile are synced between devices. Leaving open to verify once the fix reaches beta and aurora.
status-firefox17: --- → verified
Keywords: qawanted

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Duplicate of this bug: 775874
https://hg.mozilla.org/releases/mozilla-aurora/rev/1580771f753b
https://hg.mozilla.org/releases/mozilla-beta/rev/f14af298de6d
status-firefox15: affected → fixed
status-firefox16: affected → fixed
Keywords: checkin-needed

Updated

5 years ago
Status: RESOLVED → VERIFIED
Unable to reproduce the issue on:

Firefox Mobile 16.0b5 / Firefox Mobile 15
Samsung Galaxy R (Android 2.3.4)

Marking as verified on Firefox Mobile 16 and 15
status-firefox15: fixed → verified
status-firefox16: fixed → verified
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.