449388 - Custom bookmarks are duplicated when upgrading a Firefox 2 distro to Firefox 3

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[u217243]

Attachment: screenshot

Yahoo! JAPAN QA team reported that when upgrading their Firefox 2 distro to Firefox 3, the custom bookmarks were duplicated.

We have to fix distribution.js to check if the bookmarks were already added by the former DEX.

Comment 1

[Kev Needham [:kev]]

Is it that they're duplicated, or that the upgrade maintains the DEX as well as the distribution.ini?

Comment 2

[u217243]

The DEX for Firefox 2:
* add "Yahoo! JAPAN" and "My Yahoo!"
* set "cck.yahoo.initialized" pref to true

The distribution.js in Firefox 3:
* add the same bookmarks (but they may be different in other distros)
* set "distribution.yahoo-jp.bookmarksProcessed" pref to true

A possible solution is to use isBookmarked method or something:
http://developer.mozilla.org/en/docs/Manipulating_bookmarks_using_Places#Checking_to_see_if_a_URI_is_bookmarked

Comment 3

[Kev Needham [:kev]]

heya, just to clarify - is the problem caused by the dex being there? i.e. are they running it with check compat off and the dex installed, so there's actually two places where it's defined?

Comment 4

[u217243]

Sorry for confusing...

Compat check is enabled and the DEX is disabled when upgrading to Firefox 3. The problem is caused by distribution.js that doesn't check "cck.yahoo.initialized" pref nor the existing bookmarks.

Comment 5

[u217243]

Attachment: patch

This prevents distribution.js from re-adding the same bookmarks.

Comment 6

[Dan Mills [:thunder]]

That patch would prevent distribution.js from adding already-existing bookmarks, even if they are bookmarked anywhere (e.g., unsorted bookmarks).  I think in general our partners expect customized bookmarks to appear in the toolbar.

I don't understand why you have both the DEX and Firefox 3 installed.  The DEX is installed in the appdir, so it *should* have been removed during the upgrade.  Is it possible the DEX was installed manually to Firefox 2 for testing?  In that case, it would be in the profile, so it would remain after the upgrade.

The real bug here is that you have a Firefox 3 install with a Firefox 2 DEX enabled.  That is not a supported configuration.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Kev: is this needed for Major Update? Sounds like it might be INVALID ...

Comment 8

[Kev Needham [:kev]]

I think we need to replicate the issue internally. It's unclear at this point whether there is an issue or not. I'll try and do that before cob today.

Comment 9

[Dan Mills [:thunder]]

For MU, we should make 100% sure that the DEX is removed (or at least disabled) after MU.  So while the build team should look out for this problem, this bug does not block MU imo.

Comment 10

[Kev Needham [:kev]]

Kohei, could you please email location of the 3.x build this bug was reported against. I'm trying to replicate, and notice the build is no longer available. Emailing me directly with the location is fine.

Comment 11

[Kev Needham [:kev]]

Confirmed. Bookmarks in the Toolbar remain after the DEX is removed. Looks like when the DEX is loaded, the bookmarks are copied from the DEX to the user's bookmark list (suspect that's what the mozilla.partner.id is used for with custom bookmarks). Removing the DEX has no effects on bookmarks.

Comment 12

[Dan Mills [:thunder]]

Oh, of course... I understand.  The bookmarks stay behind (duh!).  Mike, this means it may have MU impact after all, my comment #9 is irrelevant (it doesn't matter if the DEX is enabled or not, the old bookmarks are still there).

But, I'm still not sure we want the patch from comment #5.  It would make it so we end up with different final states depending on which bookmarks the user has at the beginning.  That would make it more difficult to test, and would likely enable scenarios like:

menu:
unfiled:
  foo.com (starred by the user)
  foo.com/blah (starred by the user)
toolbar:
  Foo/ (created by us)
    empty! we didn't create foo.com or foo.com/blah because they already exist.

In general, I think a few duplicates resulting from the user starring pages is probably OK.  The only problem is with the extreme case (like this bug).  Perhaps we can limit the impact to updates only?

Comment 13

[Carsten Book [:Tomcat]]

Restoring MU? Whiteboard per comment #12

Comment 14

[Dan Mills [:thunder]]

How about we check for the existing of *any* CCK prefs (at all), and skip bookmarks creation in that case?

It's a hack, but I dislike it the least so far.

Another variation of the above is to allow the distribution.ini to contain an alternate pref for bookmarks (which in this case we would set to cck.whatever).

Comment 15

[Mike Connor [:mconnor]]

Yeah, that makes the most sense.  I don't think it makes sense to add bookmarks on major update, we don't do that for our own bookmarks, it only really makes sense on initial install.

Comment 16

[Dan Mills [:thunder]]

(In reply to comment #15)
> Yeah, that makes the most sense.  I don't think it makes sense to add bookmarks
> on major update, we don't do that for our own bookmarks, it only really makes
> sense on initial install.

I agree in spirit, but I don't think we can key off of e.g. browser.startup.homepage_override.mstone being set (or similar way to determine if this is first-run), because we'd want the bookmarks to be added if the user installs the customized distro on top of an existing install (with an existing profile).

So the hack from comment #14 would break in that scenario somewhat.  If we skipped bookmarks creation based on the presence of any cck prefs, we could end up skipping bookmarks when the user switches from one ff2 distribution to a *different* ff3 one (or if they happen to have a cck extension installed for any reason).  They are edge cases, but possible nonetheless.

The variation from comment #14 would fix that somewhat, the distribution.ini could contain a specific pref to key off of, so we could set that to a specific cck pref.

Yet another option is to attempt to write some kind of generic method of checking whether the bookmarks are already there where we expect them--a sort of more elaborate version of Kohei's patch.  But I fear this option a bit, it sounds harder to get right (and thus more prone to have bugs).

Sorry for the rant... just want to make sure we all understand the options here.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #16)
> I agree in spirit, but I don't think we can key off of e.g.
> browser.startup.homepage_override.mstone being set (or similar way to determine
> if this is first-run), because we'd want the bookmarks to be added if the user
> installs the customized distro on top of an existing install (with an existing
> profile).

For what it's worth, the mstone code already differentiates "new profile" and "new build with existing profile":

http://hg.mozilla.org/mozilla-central/index.cgi/annotate/c321f9d28355/browser/components/nsBrowserContentHandler.js#l117

It does that in a way that's hard to use outside of that component, though - perhaps we should make it more accessible somehow.

Comment 18

[Dan Mills [:thunder]]

(In reply to comment #17)
> (In reply to comment #16)
> > I agree in spirit, but I don't think we can key off of e.g.
> > browser.startup.homepage_override.mstone being set (or similar way to determine
> > if this is first-run), because we'd want the bookmarks to be added if the user
> > installs the customized distro on top of an existing install (with an existing
> > profile).
> 
> For what it's worth, the mstone code already differentiates "new profile" and
> "new build with existing profile":

My point was that we may want to add bookmarks in both cases, because installing a customized distro over a vanilla one needs to trigger the bookmarks to be added, as would installing a customized distro and making a new profile.

If there was a way of detecting that an install happened (vs an upgrade), that would do the trick.  But afaik there isn't (right?)

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Still not blocking major update as it only affects full installs, not MAR based updates. Apparently we're OK on partner builds as well. The only thing that this would affect would be full partner build install over existing partner build install. So, uh, PBIoPBI+ ?

And yes, doobers are needed.

Comment 20

[Dan Mills [:thunder]]

(In reply to comment #19)
> Still not blocking major update as it only affects full installs, not MAR based
> updates. Apparently we're OK on partner builds as well. The only thing that
> this would affect would be full partner build install over existing partner
> build install. So, uh, PBIoPBI+ ?
> 
> And yes, doobers are needed.

I believe this bug affects full installs over an old profile *and* MAR updates (both partial and full).

However, it is of course possible to delay MU for partners only (we have done this before, we just need to place a null update in the partner channel, and that way it will stop and not fall back to the general release channel).  So this issue need not block all of MU.

Comment 21

[Dan Mills [:thunder]]

Attachment: Configurable bookmarks addition pref v1

Allow the distribution.ini to specify which pref to use to determine if bookmarks should be added at startup or not.

Gavin, mind reviewing this?

Comment 22

[Dan Mills [:thunder]]

Comment on attachment 334022 [details] [diff] [review]
Configurable bookmarks addition pref v1

Sigh, this patch is obviously broken.

New patch coming up, sorry for the spam.

Comment 23

[Dan Mills [:thunder]]

Attachment: Configurable bookmarks addition pref v2

Second try.  Works fine on my tests.  Simply add:

[Global]
bookmarks.initialized.pref="name.of.pref"

And it will use that instead of the default.

Note: there is currently no [Bookmarks] section, so I added it to [Global].

Comment 24

[Carsten Book [:Tomcat]]

(In reply to comment #20)
oth partial and full).
> 
> However, it is of course possible to delay MU for partners only (we have done
> this before, we just need to place a null update in the partner channel, and
> that way it will stop and not fall back to the general release channel).  So
> this issue need not block all of MU.
> 

Filed Bug 450863 for this.

Comment 25

[u217243]

(In reply to comment #23)
> Created an attachment (id=334027) [details]
> Configurable bookmarks addition pref v2

Added the following line to distribution.ini:
bookmarks.initialized.pref=cck.yahoo.initialized

The patch works for me.

Comment 26

[Samuel Sidler (old account; do not CC)]

Comment on attachment 334027 [details] [diff] [review]
Configurable bookmarks addition pref v2

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 27

[Samuel Sidler (old account; do not CC)]

Dan, let's get this landed asap.

Comment 28

[Dan Mills [:thunder]]

Landed:

Checking in distribution.js;
/cvsroot/mozilla/browser/components/distribution.js,v  <--  distribution.js
new revision: 1.4; previous revision: 1.3
done

Comment 29

[Dan Mills [:thunder]]

Committed to mozilla-central:

changeset:   18394:428598e9d939
tag:         tip
user:        Dan Mills <[e-mail]>
date:        Mon Aug 25 14:24:03 2008 -0700
summary:     Bug 449388: allow the customized-bokmarks-added preference to be changed (for compatibility with legacy deployments). r=gavin

Comment 30

[Carsten Book [:Tomcat]]

verified fixed for 1.9.0.2 using the Yahoo and Green Goo Builds and an Update to Firefox 3.0.2. Custom Bookmarks work now as expected and no duplicates after Update !

Comment 31

[Carsten Book [:Tomcat]]

reopen, in the pave-over case this patch does not work and you still have duplicate bookmarks

Comment 32

[Dan Mills [:thunder]]

what is the pave-over case?

Comment 33

[Carsten Book [:Tomcat]]

Did some more tests and:

Pave over from 3.0.1 -> 3.0.3 duplicates Bookmarks
Pave over from 3.0.3 -> 3.0.3 does not duplicate Bookmarks

Comment 34

[Dan Mills [:thunder]]

Is the 3.0.3 build the same in all cases? It's important what the contents of the distribution.ini are, since it needs to be manually set depending on what it's expected to be installed on top of.

Also, if the bookmarks are already duplicated before 3.0.3 is installed on top, they will of course stay duplicated.

Comment 35

[Dan Mills [:thunder]]

Let me clarify comment #34:

* 3.0.1 doesn't support setting the pref name, so it'll use the default.
* 3.0.3 does support that, but to have it upgrade a 3.0.1 build it needs to be unset/set to the default (since that's what 3.0.1 uses).

Therefore, if you have deployed a mix of distributions that use different prefs (e.g., 2.x and 3.0.1) there is no way to upgrade without duplicating either one group or the other.

Furthermore, if we ever push an update to the distribution.ini via AUS, the upgrade may cause duplicated bookmarks for some group of people (depending on configuration).

Really, the best solution was not to have shipped any 3.0.1 partner builds :(

Comment 36

[Kev Needham [:kev]]

Hey Dan,

Is there a way we can overwrite existing, duplicate bookmarks rather than just not applying if they already exist? We've come across a scenario where a bookmark is updated with a new link to replace a broken one, and it'd probably be a better experience to replace the existing link with what's in distribution.ini, rather than just skipping it.

Comment 37

[Dan Mills [:thunder]]

The only way that I can think of atm that would be reliable would be to have the distribution.ini contain some kind of unique identifier for each bookmark, which would kind of suck to maintain.

Even with a unique ID, though, I would want to be pretty careful when modifying bookmarks... unlike other customizations, users are expected (encouraged, even) to modify their bookmarks.  We would not want to stomp on the user's changes, ever.

For links that change routinely a livemark is best.

In any case, this discussion is orthogonal to this bug (unless you want to use the patch from comment #5).

Comment 38

[Kev Needham [:kev]]

I hardly think it's orthogonal, since it's an unintended consequence of the patch. Updates to bookmark links are not regular, so a livemark doesn't really make sense (and implementation of that is definitely orthogonal ;) ), but they do happen (change of branding, properties, etc.).

Not a huge deal, just something to think about.

And with that, I think it's safe to close this out, unless Tomcat disagrees, as we'll do some re-evaluation of what places does vs. potential needs and come back to it, and the patch as it stands addresses the lion's share of use cases.

Comment 39

[Carsten Book [:Tomcat]]

don't disagree :) marking this bug back as fixed per comment #35 and #38

Comment 40

[Dan Mills [:thunder]]

Hey Kev,

I just wanted to separate the "allow bookmarks to be modifiable over time" discussion from this bug, since It's not a feature we had before.

I suggest we move that discussion to email.

Comment 41

[Kev Needham [:kev]]

Agreed. :) I'll bring the doober, which has been acquired..

Comment 42

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I doubt we're going to add it to the test suite now, if we haven't yet...