957131 - Synced bookmarks appear in guest mode but not in default profile

Status: VERIFIED FIXED

(Firefox for Android :: General, defect, major)

Description

[u421692]

Environment:
Device: Samsung Galaxy Tab (Android 4.0.4)
Build: Firefox 27 Beta 4 / Firefox 26.0.1

Steps to reproduce:
1. Set up Sync in a profile. Wait for sync to finish.
2. Open Firefox. Switch to guest mode.
3. Add a bookmark on desktop.
4. Back on the phone, app-switch back to settings, leaving Firefox running. Menu > Sync Now. Wait until the spinner stops.

Expected result:
The new bookmark doesn't appear in the guest profile and appears in the default profile.

Actual result:
The new bookmark appears in the guest profile and doesn't appear in the default profile.

Comment 1

[Richard Newman [:rnewman]]

Sounds like BrowserProvider is writing to the wrong place, perhaps due to GeckoProfile shenanigans that were fixed in 28. Wes, any thoughts?

Comment 2

[Wesley Johnston (:wesj)]

Not sure what's going on, but I'll take this.

Comment 3

[Wesley Johnston (:wesj)]

I see this on nightly too. From some logging, it looks like Sync isn't requesting that we use a particular profile, so we just sync to the current one:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java#73

Can we throw a DEFAULT_PROFILE on the URI's in here?

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Wesley Johnston (:wesj) from comment #3)
> I see this on nightly too. From some logging, it looks like Sync isn't
> requesting that we use a particular profile, so we just sync to the current
> one:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/sync/
> repositories/android/AndroidBrowserBookmarksDataAccessor.java#73

I wondered when this was going to bite us.  I thought Sync was careful to set DEFAULT_PROFILE everywhere, but code reading suggests not.

Comment 5

[Richard Newman [:rnewman]]

I think we used to assume that no argument = "default", not 'current', and of course it never mattered until Guest Mode came along. We never did the work to make anything multi-profile aware (e.g., see Bug 707123 Comment 2 and onward).

Engineering debt bites again! :D

Comment 6

[Aaron Train [:aaronmt]]

Please consider (if necessary of course) 'other' profiles associated with installed synthesized web-apps.

Comment 7

[Richard Newman [:rnewman]]

(In reply to Aaron Train [:aaronmt] from comment #6)
> Please consider (if necessary of course) 'other' profiles associated with
> installed synthesized web-apps.

Potentially falls into the same bucket. Depends on how independent the synthesized web apps are -- if they fire an intent at Fennec with a profile, then they're not independent at all.

If we're going to ship multi-profile support -- whether that be real profile switching, or simpler UI for profile switching (guest mode and apps) -- then we need to implement multi-profile support. That means SharedPreferences (Bug 940575), Sync, FHR and Product Announcements (Bug 896505), etc. etc.

That's a project, not a bug.

Comment 8

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #4)

> I wondered when this was going to bite us.  I thought Sync was careful to
> set DEFAULT_PROFILE everywhere, but code reading suggests not.

As far as I can tell, we don't ever set PARAM_PROFILE ("profile") on any ContentResolver URI, so this isn't an individual omission.

I did some sleuthing, and it seems like my assumption in Comment 5 is long-lived: e.g., Bug 880599 Comment 8. All of Sync has always assumed that GeckoProfile will treat "no profile argument" as "default", not "current". This does not just affect bookmarks.

My guess is that that assumption changed (or was invalidated) in GeckoProfile when the guest profile work happened.

I added some QA steps in Bug 871863 Comment 11 that should have caught this regression in 25. This should be in moztrap/litmus/whatever it's called these days, and should probably be a routine smoketest. Setting a bunch of flags so that somebody does that.

We have two options:

* Revert the behavior of GeckoProfile, and make sure that Fennec always specifies its current active profile when making CR requests.
* Implement the profile argument stuff for all of Sync's database accessors, which might also solve Bug 896109.

My general feeling is that defaulting to the current profile in GeckoProfile makes sense (albeit with edge cases), so option two is the longer but more thorough and correct choice. But that will be a potentially sizable uplift which might not be suitable for 27.

Anyone else have an opinion?

Comment 9

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Feel free to review this if you love it, but I need to test, and I wanted feedback on how you'd like to handle the constants before I did more. This fixes up everything in /sync that uses a CONTENT_URI that I could find. I wanted a constant for "default", so I put one in GeckoProfile. I have a feeling you'll want it somewhere else, or to at least mirror it in the sync repo for ease-of-testing?

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8359502 [details] [diff] [review]
WIP Patch

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

Yeah, this'll break the upstream build. Fortunately we already have this:

src/main/java/org/mozilla/gecko/sync/setup/Constants.java
17:  public static final String DEFAULT_PROFILE = "default";

which you should use instead of GeckoProfile.DEFAULT_PROFILE for /sync/. (You actually changed some uses of this constant.)

You'll need to be a little more judicious about switching things over, so this patch'll shrink a lot, but the core of the change -- BrowserContractHelpers -- looks like the right direction to me.

::: mobile/android/base/BrowserApp.java
@@ +119,5 @@
>      private HomePager mHomePager;
>      private View mHomePagerContainer;
>      protected Telemetry.Timer mAboutHomeStartupTimer = null;
>      private ActionModeCompat mActionMode;
> +    private boolean mShowActionModeEndAnimation = false;

I think your patch stack is dirty…

::: mobile/android/base/GeckoApp.java
@@ +1148,5 @@
>      {
>          GeckoAppShell.registerGlobalExceptionHandler();
>  
>          // Enable Android Strict Mode for developers' local builds (the "default" channel).
> +        if (GeckoProfile.DEFAULT_PROFILE.equals(AppConstants.MOZ_UPDATE_CHANNEL)) {

Over-zealous find and replace, good sir. This refers to the default update channel.

@@ +1181,5 @@
>                      }
>                      if (profileName == null) {
>                          profileName = getDefaultProfileName();
>                          if (profileName == null)
> +                            profileName = GeckoProfile.DEFAULT_PROFILE;

This use of DEFAULT_PROFILE is fine, or you can use Sync's constant. Doesn't matter to me.

::: mobile/android/base/home/ListPage.java
@@ +152,5 @@
>              final ContentResolver cr = getContext().getContentResolver();
>  
>              // XXX: Use the test URI for static fake data
>              final Uri fakeItemsUri = HomeListItems.CONTENT_FAKE_URI.buildUpon().
> +                appendQueryParameter(BrowserContract.PARAM_PROFILE, GeckoProfile.DEFAULT_PROFILE).build();

I don't think this is correct for guest mode. You should be explicitly leaving UI code *without* a profile parameter.

::: mobile/android/base/sync/receivers/SyncAccountDeletedService.java
@@ +102,5 @@
>      // extract it by hand. We're not worried about the Account being
>      // deleted out from under us since the prefs remain even after Account
>      // deletion.
>      final String product = GlobalConstants.BROWSER_INTENT_PACKAGE;
> +    final String profile = GeckoProfile.DEFAULT_PROFILE;

This was already correct :D

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Know your busy. Throw to someone else if you want (liuche?). WFM though. Tried to force sync multiple times and only got new things in my default profile.

Comment 12

[Richard Newman [:rnewman]]

Comment on attachment 8360089 [details] [diff] [review]
Patch v1

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

This looks good. Thanks for taking care of this, Wes.

The Sync changes need to be applied upstream when you land this. liuche, mcomella, or myself can take care of that, if you don't have a checkout of android-sync; just poke us when you're ready.

Comment 13

[Richard Newman [:rnewman]]

I took care of the uplift. (Relaxing evening work!)

Pull request to merge:

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

Comment 14

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/124f393d9f2e
https://github.com/mozilla-services/android-sync/commit/f16646beee63a47ccbb5587c9c32e442b3ce7a85

Comment 15

[Aaron Train [:aaronmt]]

in-testsuite typically reserved for automated tests

Comment 16

[Wesley Johnston (:wesj)]

Comment on attachment 8360089 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Sync forever + new Guest mode (bug 871863)
User impact if declined: Guest mode picks up your private info
Testing completed (on m-c, etc.): Landed on mc today. Tested locally.
Risk to taking this patch (and alternatives if risky): Pretty low risk. Just adding a param to some UI's
String or IDL/UUID changes made by this patch: None.

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/124f393d9f2e

Comment 18

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e3ed25347ea3

Comment 19

[Richard Newman [:rnewman]]

Wes: this didn't apply cleanly to Beta, so I didn't land it there, and I don't have the time to clean it up now. Please land it if you get the chance!

Comment 20

[u421692]

Verified as fixed on latest Aurora and Nightly(2014-01-16). 
Marking bug as verified only after uplifting to Beta.

Comment 21

[u421692]

Test is allready in moztrap: https://moztrap.mozilla.org/manage/case/9220/

Comment 22

[Wesley Johnston (:wesj)]

All our test files went from *.java.in to *.java between beta and aurora. Fixed up (and removed the GeckoProfile references since it isn't imported when tests are built there either):

https://hg.mozilla.org/releases/mozilla-beta/rev/b7d7ea54a6d2

Comment 23

[Richard Newman [:rnewman]]

Thanks, Wes!

Comment 24

[Paul Feher]

Verified as fixed on Firefox for Android 27 Beta 8