Closed Bug 957131 Opened 6 years ago Closed 6 years ago

Synced bookmarks appear in guest mode but not in default profile

Categories

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

27 Branch
All
Android
defect
Not set
major

Tracking

()

VERIFIED FIXED
Firefox 29
Tracking Status
firefox26 --- wontfix
firefox27 + verified
firefox28 --- verified
firefox29 --- verified
fennec 27+ ---

People

(Reporter: u421692, Assigned: wesj)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
tracking-fennec: --- → ?
Flags: needinfo?(rnewman)
Sounds like BrowserProvider is writing to the wrong place, perhaps due to GeckoProfile shenanigans that were fixed in 28. Wes, any thoughts?
Flags: needinfo?(rnewman) → needinfo?(wjohnston)
Severity: normal → major
Not sure what's going on, but I'll take this.
Assignee: nobody → wjohnston
Flags: needinfo?(wjohnston)
tracking-fennec: ? → 27+
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?
Flags: needinfo?(rnewman)
(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.
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
Flags: needinfo?(rnewman)
Please consider (if necessary of course) 'other' profiles associated with installed synthesized web-apps.
(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.
(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?
Flags: in-testsuite?
Flags: in-qa-testsuite?
Flags: in-moztrap?
Attached patch WIP Patch (obsolete) — Splinter Review
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?
Attachment #8359502 - Flags: feedback?(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
Attachment #8359502 - Flags: feedback?(rnewman)
Attached patch Patch v1Splinter Review
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.
Attachment #8360089 - Flags: review?(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.
Attachment #8360089 - Flags: review?(rnewman) → review+
Attachment #8359502 - Attachment is obsolete: true
Whiteboard: [needs porting to android-sync]
I took care of the uplift. (Relaxing evening work!)

Pull request to merge:

https://github.com/mozilla-services/android-sync/pull/384
Status: NEW → ASSIGNED
Whiteboard: [needs porting to android-sync]
in-testsuite typically reserved for automated tests
Flags: in-qa-testsuite?
Flags: in-moztrap?(mihai.g.pop)
Flags: in-moztrap?
Keywords: verifyme
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.
Attachment #8360089 - Flags: approval-mozilla-beta?
Attachment #8360089 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/124f393d9f2e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Attachment #8360089 - Flags: approval-mozilla-beta?
Attachment #8360089 - Flags: approval-mozilla-beta+
Attachment #8360089 - Flags: approval-mozilla-aurora?
Attachment #8360089 - Flags: approval-mozilla-aurora+
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!
Hardware: ARM → All
Verified as fixed on latest Aurora and Nightly(2014-01-16). 
Marking bug as verified only after uplifting to Beta.
Test is allready in moztrap: https://moztrap.mozilla.org/manage/case/9220/
Flags: in-moztrap?(mihai.g.pop) → in-moztrap+
Keywords: verifyme
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
Verified as fixed on Firefox for Android 27 Beta 8
Status: RESOLVED → VERIFIED
Duplicate of this bug: 958084
Assignee: wjohnston → nalexander
Assignee: nalexander → wjohnston
You need to log in before you can comment on or make changes to this bug.