1380998 - Fennec Bookmark Management: sync after editing folders doesn't display the new name

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Sorina Florean [:sflorean]]

Environment: 
Device: 
- Asus Zenpad 8(Android 6.0.1);
- Motorola Nexus 6 (Android 7.0)
Build: Nightly 56.0a1 (2017-07-13);
Prerequisites: log in with the same account on both devices and have one folder TEST.

Steps to reproduce:
1. On device 1 long tap on "Test" and change the name to "Mozilla";
2. Add 2-3 new bookmarks in folder "mozilla";
3. Perform a sync on device 1;
4. Perform a sync on device 2 and go to folder "Mozilla".

Expected result:
Folder "Mozilla" is displayed in Bookmarks Panel and the new bookmarks are listed in folder.

Actual result:
- The name of folder is still "Test".
- The new bookmarks are listed in folder.

Notes: syncing between desktop and android works fine, the name is changed.

Comment 1

[Jing-wei Wu [:jwu]]

Hi Sorina,

I cannot reproduce this issue on Nightly (2017-07-16), my devices are
- Nexus 5 (Android 6.0.1), and 
- Pixel C (Android O)

Could you help check this issue with latest Nightly?
Also since this is a sync issue, could you enable debug mode to see the device log if it shows any error message?

To enable debug mode: 
1. Launch Nightly, go to settings and click your email to enter Sync page;
2. Click 'More...' button and you should see 'Enable Debug Mode' on top right, enable the checkbox.

Then start reproducing this issue, and get device report in device Settings/Developer options, hope the information could help us find the root cause.

Comment 2

[Sorina Florean [:sflorean]]

Attachment: bugreport-2017-07-17-11-08-00.zip

Try these steps:
1. log in with the same account on devices;
2. on device 1 make a folder; and sync both devices;
3. change the folder name on device 1 and sync again. 
My account has more than 15 folders and 450 bookmarks. Also, isn't reproducible all the time. 

If there is something I can do, another log, let me know.

Comment 3

[Richard Newman [:rnewman]]

Android uses timestamps for change tracking. If you make a change to a bookmark during a sync, at just the right time, it will be ignored. That seems to be the most likely culprit here. Bug 1364644 should address that.

Grisha, what do you think?

Comment 4

[:Grisha Kruglov]

Given the inconsistent nature of what Sorina is seeing, bad interaction between timestamps and sync does seem pretty likely (see https://bugzilla.mozilla.org/show_bug.cgi?id=1343985#c1 for some discussion on this).

If device is rooted and has shell access, we can probably say for certain by looking at the last-synced timestamps stored in shared prefs and the modified timestamps in the browser.db file. In lieu of that, we can at least try some "litmus tests".

If it is a timestamp sync tracking problem, a missed change is "missed forever", until another action prompts sync to upload this record. And so we can try some scenarios which should prompt an upload of the record:
- once you observe a missing change after a sync, can you try syncing a bunch of times - without making any further changes on either device - and see if that "fixes" the problem?
--> I expect that it should not.

- once you observe this problem for a bookmark folder, can you place another bookmark into that folder (either a normal bookmark, or a nested folder), sync, and see if both changes (re-parenting and your original renaming) made it onto the other device?
--> I expect that unless you hit the same bug again (in which case, it's possible your bookmark tree will get corrupted), this should "fix" the problem.

- you should be able to reproduce this problem for android->desktop as well as android->android. When you're testing, to control for causes, ensure you're not making any modifications to desktop bookmarks, because that might trigger a re-upload of android bookmarks.
--> I expect that with some effort, this bug should be reproducible for any combination of device<->device syncing.

Tests above are not conclusive, but if you observe "unexpected" behaviour, something else could also be at play.

Additionally, I would expect the timestamp issue to be somewhat infrequent and relatively hard to reproduce during "normal" usage of the browser (e.g. without actively trying to cause a clash of local changes and a sync). If you're hitting it consistently and with ease, something else could be happening.

Additionally, during a sync we log detailed information about how many records were sent, received, merged, etc. Looking at these numbers will help narrow down the cause as well. If you attach output of adb logcat | grep FxAccounts, that would help a lot.
Look for log messages such as "ServerSyncStage :: Stage bookmarks received 2; stored 2, reconciling 0 and failed to store 0. Sent 1; server accepted 1 and rejected 0. Duration: 0.26 seconds."

Comment 5

[:Grisha Kruglov]

Could you try running through Comment 4?

Comment 6

[Sorina Florean [:sflorean]]

Could you be more specific, maybe some STR? Thanks

Comment 7

[Wesly Huang (EPM)]

Hi Sorina,

Per Grisha's assumptions and reply in the mail "[feature][mobile]Fennec Bookmark Management - Pre-Beta sign-off (Yellow)", this issue will be resolved after version-based sync is implemented, which is done on 1364644. Would you mind re-test this issue now with the latest nightly?

Thanks!

Comment 8

[Sorina Florean [:sflorean]]

(In reply to Wesly Huang (Fennec Frontend/Firefox EPM) from comment #7)
> Hi Sorina,
> 
> Per Grisha's assumptions and reply in the mail "[feature][mobile]Fennec
> Bookmark Management - Pre-Beta sign-off (Yellow)", this issue will be
> resolved after version-based sync is implemented, which is done on 1364644.
> Would you mind re-test this issue now with the latest nightly?
> 
> Thanks!

Sorry for the late reply. Tested again with Asus ZenPad 8 (Android 6.0.1) and LG G4 (Android 5.1) on latest Nightly build from 08/28 and the issue is still reproducible. I was following the steps from description and comment 2.

Comment 9

[Nicole Yee (:nicoleyee)]

Wesly, can you please investigate further with the engineering team?

Comment 10

[:Grisha Kruglov]

Another likely cause is Bug 1392505. Its symptoms could be summarized as "missing updates made on other sync devices".

I'm playing around with this locally, and certainly observing weird/inconsistent behavior when editing same parts of the bookmark tree on different Android devices.

Comment 11

[Wesly Huang (EPM)]

Grisha, since Bug 1392505 is fixed now, do you think it's worth testing it again before the team has the bandwidth (after 57) to look into it?

Comment 12

[Wesly Huang (EPM)]

Sorry Sorina, do you mind helping check if this is still reproducible? Thanks.

Comment 13

[:Grisha Kruglov]

This slipped off my radar - yeah, let's re-test this. It's possible we still have other issues that might look similar, but Bug 1392505 should have helped in a lot of cases.

Comment 14

[Sorina Florean [:sflorean]]

Tested on latest Nightly build with different Android versions: Samsung Galaxy Note 4 (Android 5.0.1), Oneplus Two (Android 6.0.1), Motorola Nexus 6 (Android 7.1.1).
Following the description and comment 2 the issue is not reproducible, with two devices. But performing the steps: rename a folder on 2 devices, perform sync on all devices, check device 3: the new folder name was not displayed. I don't think that is related to Android versions because when the steps were performed on two devices, the new folder names was displayed correctly.

Comment 15

[Wesly Huang (EPM)]

Thanks, Sorina, just to clarify the user scenario here. For "rename a folder on 2 devices", you mean rename device A's folder to "ABC", then device B's folder to "ABC" as well? (unsure about the purpose as a user to do it) 

(In reply to Sorina Florean [:sorina] from comment #14)
> But performing the steps: rename a folder on 2 devices, perform
> sync on all devices, check device 3: the new folder name was not displayed.
> I don't think that is related to Android versions because when the steps
> were performed on two devices, the new folder names was displayed correctly.

Comment 16

[Sorina Florean [:sflorean]]

(In reply to Wesly Huang (Fennec Frontend/Firefox EPM) from comment #15)
> Thanks, Sorina, just to clarify the user scenario here. For "rename a folder
> on 2 devices", you mean rename device A's folder to "ABC", then device B's
> folder to "ABC" as well? (unsure about the purpose as a user to do it) 
> 
> (In reply to Sorina Florean [:sorina] from comment #14)
> > But performing the steps: rename a folder on 2 devices, perform
> > sync on all devices, check device 3: the new folder name was not displayed.
> > I don't think that is related to Android versions because when the steps
> > were performed on two devices, the new folder names was displayed correctly.

Sorry for confusion here, rename 2 different folders on two devices, with 2 different names.

Comment 17

[Wesly Huang (EPM)]

Grisha, would you help take a look at it? Is there any limitation about sync for 2 (or multiple) folders scenario?

Thanks. 

 (In reply to Sorina Florean [:sorina] from comment #16)
> (In reply to Wesly Huang (Fennec Frontend/Firefox EPM) from comment #15)
> > Thanks, Sorina, just to clarify the user scenario here. For "rename a folder
> > on 2 devices", you mean rename device A's folder to "ABC", then device B's
> > folder to "ABC" as well? (unsure about the purpose as a user to do it) 
> > 
> > (In reply to Sorina Florean [:sorina] from comment #14)
> > > But performing the steps: rename a folder on 2 devices, perform
> > > sync on all devices, check device 3: the new folder name was not displayed.
> > > I don't think that is related to Android versions because when the steps
> > > were performed on two devices, the new folder names was displayed correctly.
> 
> Sorry for confusion here, rename 2 different folders on two devices, with 2
> different names.

Comment 18

[Wesly Huang (EPM)]

Hi Grisha:

I feel the new symptom discovered (rename/sync 2 folders) may not a be frequent use case so not necessarily a blocker for the feature release (and maybe we should use another bug for tracking). But before seeking product's agreement I would like to hear your view of it.

During another discussion with Nick (about Gradle build) we happened to mention this issue and he thought the symptom could be more like a design choice/limitation than a bug, since when you rename multiple folders it could be hard to tell the right name-folder mapping so cause some problem. It sounds reasonable to me but I don't have insight into the actual implementation, and since Jingwei has left Mozilla I believe you are the best go-to person for this. (Nick also recommended you as the best choice)

Would you kindly share? Thanks.

Comment 19

[:Grisha Kruglov]

Hi Wesly,

I concur with Nick in that there remain some implementation limitations which are inherent to current bookmark sync on Android, which will manifest in strange behaviour in certain edge cases.

While we've removed a lot of the "things might go quite wrong" cases with the bookmark version tracker (Bug 1364644), fixing timestamp bugs (Bug 1392505), the atomic uploader work (Bug 1253111), buffering (Bug 1291821) and fully enabling batching on the servers, the logic around merging together bookmark trees from different devices didn't significantly change yet on Android, and there remain these inherent flaws. We're moving in the right direction though.

The current plan is to incrementally patch enough of these concrete problems, whenever feasible, while in the meantime focus on the overall sync complexity and perf improvements (Bug 1408243).

Likely related bookmark merging bug: Bug 1343365. Also, see Bug 1353641 for a broader take on this.

Addressing most of the merging edge cases properly looks quite similar to Bug 1305563, and will take a good while to actually ship.

Overall, considering how I imagine most people will use bookmark management on Android, we should be able to live with these edge cases, at least for some time. By now we've addressed the major data corruption risks that could have _really_ screwed up bookmarks, and we're left with more minor risks that _might_ screw up your bookmarks in limited ways following very specific types of edits in multi-device environment.

Utility of having proper bookmark management, which is quite likely to "just work" for most people's flows (I hope, assuming most flows are straightforward) - and will always work if user isn't using Sync at all - to me greatly outweighs these various concerns.

My vote is to let this ride the trains for 58, while also working to improve experience around specific edge cases. Some of these improvements might make it to 58, some will come later.

Comment 20

[Wesly Huang (EPM)]

Thanks Grisha for the comment with a bigger picture of sync's status and plan, learned a lot and personally I'm quite convinced there is no reason to wait longer for releasing bookmark mgt thus propose to let it ride 58. To me 2 things remaining should be addressed before moving on:

1. Joe, would you help comment from Product's perspective?

2. Sorina, would like to know your view of it as well and if we'll be able to include this feature in the pre-beta sign-offs? (as we didn't plan it firmly in the beginning of 58)

Thanks.

Comment 21

[Sorina Florean [:sflorean]]

(In reply to Wesly Huang (Fennec Frontend/Firefox EPM) from comment #20)
> Thanks Grisha for the comment with a bigger picture of sync's status and
> plan, learned a lot and personally I'm quite convinced there is no reason to
> wait longer for releasing bookmark mgt thus propose to let it ride 58. To me
> 2 things remaining should be addressed before moving on:
> 
> 2. Sorina, would like to know your view of it as well and if we'll be able
> to include this feature in the pre-beta sign-offs? (as we didn't plan it
> firmly in the beginning of 58)
> 
> Thanks.

Hi Wesly, from my side it's ok to let the feature in 58. We can include also for pre-beta sign-off, since we have 1/2 weeks to test till then. Thanks

Comment 22

[Wesly Huang (EPM)]

Hi Sorina:

Sorry but a further request before the decision. Would you help take a video of the latest symptom mentioned in comment 14/16? During the discussion with Joe we have different interpretations of the description and believe a video will greatly help. Guess it's unlikely to change our current thought (go for 58) but would like to make it clear. Thanks.

Comment 23

[Sorina Florean [:sflorean]]

Hi all,

Here is a video with the issues: https://youtu.be/SKXeAH-X-Mk. I just perform the steps on two devices since the issue is reproducible on latest Nightly build.
 Notes: 
-the issue is only reproducible on Android-Android devices, not on Android-Desktop sync;
-only applicable to folders that are new. In video I renamed a folder made yesterday and the new name was synced;

Comment 24

[Joe Cheng [:jcheng] (please needinfo)]

Thank you Sorina
It requires quite some steps to reproduce the bug!
Given your additional comments on the bug being only reproducible on Android-Android and only applicable to folders that are new. I agree the risk is low. Let's have this feature ride on 58. Thanks all!

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[triage] Bulk edit from title: this is a non-critical issue. Please remove priority if you wish this to be re-triaged.

Comment 26

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.