Closed
Bug 957131
Opened 12 years ago
Closed 12 years ago
Synced bookmarks appear in guest mode but not in default profile
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox26 wontfix, firefox27+ verified, firefox28 verified, firefox29 verified, fennec27+)
VERIFIED
FIXED
Firefox 29
People
(Reporter: u421692, Assigned: wesj)
References
Details
Attachments
(1 file, 1 obsolete file)
18.46 KB,
patch
|
rnewman
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
tracking-fennec: --- → ?
Flags: needinfo?(rnewman)
Comment 1•12 years ago
|
||
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)
Updated•12 years ago
|
Severity: normal → major
Updated•12 years ago
|
tracking-firefox27:
--- → ?
Assignee | ||
Comment 2•12 years ago
|
||
Not sure what's going on, but I'll take this.
Assignee: nobody → wjohnston
Flags: needinfo?(wjohnston)
Updated•12 years ago
|
Updated•12 years ago
|
tracking-fennec: ? → 27+
Assignee | ||
Comment 3•12 years ago
|
||
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)
Comment 4•12 years ago
|
||
(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•12 years ago
|
||
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)
Comment 6•12 years ago
|
||
Please consider (if necessary of course) 'other' profiles associated with installed synthesized web-apps.
Comment 7•12 years ago
|
||
(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•12 years ago
|
||
(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?
Assignee | ||
Comment 9•12 years ago
|
||
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 10•12 years ago
|
||
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)
Assignee | ||
Comment 11•12 years ago
|
||
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 12•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #8359502 -
Attachment is obsolete: true
Updated•12 years ago
|
Whiteboard: [needs porting to android-sync]
Comment 13•12 years ago
|
||
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]
Comment 14•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/124f393d9f2e
https://github.com/mozilla-services/android-sync/commit/f16646beee63a47ccbb5587c9c32e442b3ce7a85
Target Milestone: --- → Firefox 29
Comment 15•12 years ago
|
||
in-testsuite typically reserved for automated tests
Assignee | ||
Comment 16•12 years ago
|
||
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?
Comment 17•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Attachment #8360089 -
Flags: approval-mozilla-beta?
Attachment #8360089 -
Flags: approval-mozilla-beta+
Attachment #8360089 -
Flags: approval-mozilla-aurora?
Attachment #8360089 -
Flags: approval-mozilla-aurora+
Updated•12 years ago
|
Comment 18•12 years ago
|
||
Comment 19•12 years ago
|
||
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
Reporter | ||
Comment 20•11 years ago
|
||
Verified as fixed on latest Aurora and Nightly(2014-01-16).
Marking bug as verified only after uplifting to Beta.
Reporter | ||
Comment 21•11 years ago
|
||
Test is allready in moztrap: https://moztrap.mozilla.org/manage/case/9220/
Flags: in-moztrap?(mihai.g.pop) → in-moztrap+
Assignee | ||
Comment 22•11 years ago
|
||
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 24•11 years ago
|
||
Verified as fixed on Firefox for Android 27 Beta 8
Status: RESOLVED → VERIFIED
Updated•11 years ago
|
Assignee: wjohnston → nalexander
Updated•11 years ago
|
Assignee: nalexander → wjohnston
Updated•5 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•