Carsten Book [:Tomcat] Reporter
	Description • 15 years ago

Steps to reproduce:

-> Install a custom build (like a byob one)
-> Do not migrate settings from a second browser
--> Default bookmarks are missing (custom bookmarks are still there)

Does not happen when you migrate from a second browser.

	Carsten Book [:Tomcat] Reporter
	Comment 1 • 15 years ago

this is a regression between Firefox 3.5.2 and Firefox 3.5.3 , adding dietrich to this bug. Maybe we have changed something in places that cause this problem ?

	Carsten Book [:Tomcat] Reporter
	Comment 2 • 15 years ago

might be related to https://bugzilla.mozilla.org/show_bug.cgi?id=483980 ? ...at least the only places bug i found fixed for 3.5.3

	Carsten Book [:Tomcat] Reporter
	Comment 3 • 15 years ago

note: also only affects builds where pre-defined bookmarks etc exists. Builds without any customized bookmarks like Google, Personas are not affected by this bug.

	Kev Needham [:kev]
	Comment 4 • 15 years ago

Changing this over to a Firefox bug, as it's a product bug, not a RelEng issue.

Not sure where exactly the problem is, but it may be that modules/distribution.js is getting loaded before the default bookmark information is, so when the browser goes through its first-run with bookmarks defined in <appdir>/distribution/distribution.ini, defaults aren't written because they aren't yet defined.

	Marco Bonardo [:mak] Assignee
	Comment 5 • 15 years ago

could even just be that some bookmark is added before default bookmarks, and iirc if browserglue detects that, it won't try to import default bookmarks seeing the db has been already used.

	Marco Bonardo [:mak] Assignee
	Comment 6 • 15 years ago

the above bug could be the unwilling culprit, by the fact it changes the order of Places initialization.

	Marco Bonardo [:mak] Assignee
	Comment 7 • 15 years ago

related code:
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#567

	Kev Needham [:kev]
	Comment 8 • 15 years ago

Additional information on the bug:

When installing a version of Firefox that 1) contains customized bookmarks in distribution/distribution.ini and 2) is installing to a clean profile (i.e. will trigger first-run) and 3) skips the import of other browser settings through the migration wizard, the default bookmarks will not be created in either the Bookmarks Menu or the Bookmarks Toolbar.

Steps to replicate:

- Install Firefox
- In the application directory, copy the sample distribution.ini which contains customized bookmarks into a directory named distribution/
- Start Firefox with a clean profile
- Skip the import of settings from another browser in the migration wizard

Expected results:

- All default bookmarks appear on both bookmark menu and toolbar
- Bookmark titled "Test Link" appears in both bookmark menu and toolbar

Actual results:

- "Most Visited" Smart bookmark is only default bookmark to appear on toolbar
- Bookmark titled "Test Link" appears in both bookmark menu and toolbar

If you accept the import of other browser settings from the migration wizard, all default bookmarks appear.

This breaks all new installs of our custom builds w/3.5.3, so I'll be asking for blocking on 3.5.4.

	Marco Bonardo [:mak] Assignee
	Comment 9 • 15 years ago

where are handled distribution.ini contents?

	Kev Needham [:kev]
	Comment 10 • 15 years ago

Marco: modules/distribution.js

	Kev Needham [:kev]
	Comment 11 • 15 years ago

(around line 250, iirc)

	Marco Bonardo [:mak] Assignee
	Comment 12 • 15 years ago

oh so this is a module imported from browserGlue, even better. we should be able to know if distribution added anything and avoid the above check. or reorder things. Btw i'm pretty sure this runs before browserGlue and causes the above code to sneak in.

	Marco Bonardo [:mak] Assignee
	Comment 13 • 15 years ago

(In reply to comment #12)
> Btw i'm pretty sure this runs before browserGlue

i mean before Places initialization in browserGlue

	Marco Bonardo [:mak] Assignee
	Comment 14 • 15 years ago

taking for now, since i know that code.

	Daniel Veditz [:dveditz]
	Comment 15 • 15 years ago

Not sure this can realistically make next week's code freeze so not blocking 1.9.1.4, but we would like a fix for this regression.

	Marco Bonardo [:mak] Assignee
	Comment 16 • 15 years ago

This is completely untested, so i need to test it and see if i can test this automatically (i think so).
In my mind this solves a bunch of headaches i had in the past, but could even be i'm wrong.

	Marco Bonardo [:mak] Assignee
	Comment 17 • 15 years ago

Comment on attachment 401524 [details] [diff] [review]
patch v1.0

bah, looks like there is no way to notify something synchronously as soon as a service has been instantiated... that would have been really useful making our init less fragile.

Unless XPCOM will magically have a method to synchronously notify something at service instantiation, i'll probably have to just workaround this moving the bookmarks import or checking the bmimported pref.

	Marco Bonardo [:mak] Assignee
	Comment 18 • 15 years ago

patch with test.
Still not ready for review, i want to larify if we have Ts hits from distribution, and if we can reduce them.

	Kev Needham [:kev]
	Comment 19 • 15 years ago

almost positive this is not in TS. the distribution.js module needs tests defined for all the customizations within, which is on the todo list for this week (and has been on said list for months but kept getting de-prioritized until bad things happened).

	Marco Bonardo [:mak] Assignee
	Comment 20 • 15 years ago

(In reply to comment #19)
> almost positive this is not in TS.
still getting 3 times the object and parse the ini 3 times looks wrong, a getter would probably help

> the distribution.js module needs tests
> defined for all the customizations within, which is on the todo list for this
> week (and has been on said list for months but kept getting de-prioritized
> until bad things happened).

yeah i'm just testing that bookmarks are imported and in correct position, but this is not a complete test of distribution functionality.

	Marco Bonardo [:mak] Assignee
	Comment 21 • 15 years ago

I'm using a getter in browserGlue, converted some stuff to getters since we don't need everything until it is used, and used __defineGetters__. I have not used XPCOMUtils lazygetters since they are not available on 1.9.1, btw i have another patch around converting glue getters to that.

The test checks everything related to bookmarks, but i could easily check other prefs settings if we want.

Eventually if this is too much for 1.9.1 i can do a reduced patch, just moving bookmarks customization, or just add some test for other stuff in distribution.ini.

	Marco Bonardo [:mak] Assignee
	Comment 22 • 15 years ago

ps: dunno if thunder is fine as reviewer or i should ask someone other in browser, i asked him because the original file is his.

	Dan Mills [:thunder]
	Comment 23 • 15 years ago

Comment on attachment 401743 [details] [diff] [review]
patch v2.1

Patch looks good to me, modulo:

The reason I didn't make the distribution customizer a getter in the first place was because I wanted it to go out of scope and get garbage collected, since it's not used after startup.

I don't think it'll make any real difference in practice, though, since the customizer only holds refs to very common services.

Still, it might make sense to dispose of it after we don't need it.

	Marco Bonardo [:mak] Assignee
	Comment 24 • 15 years ago

fix a typo, delete the customizer after applyBookmarks

	Marco Bonardo [:mak] Assignee
	Comment 25 • 15 years ago

Comment on attachment 403226 [details] [diff] [review]
patch v2.2

oh well, actually i cannot predict reliably if applybookmarks will run before or after applyCustomizations, so i cannot dispose the object sync in browserGlue.

this could be handled internally but would add a bunch of code just to take count of what has been called and what not, just to free some reference to commonly used services, as you said.
I'm not sure if the win is worth doing that.

	Marco Bonardo [:mak] Assignee
	Comment 26 • 15 years ago

so, the problem with the previous approach (new object for every call) is that before we had less points that were going to check for distribution.ini existence (bookmarks were imported at applyCustomizations), while actually both profile init and Places init would check that, and i don't want to regress Ts, obviously.

This disposes the customizer once it has finished, asking it if it has finished. I don't like this a lot, but cannot see a better solution atm, since only the customizer can know if work is complete and i cannot rely on a call order.

plus added a further test for prefs default and minor changes.

Thunder, could you check the interdiff with v2.1 (2.2 is not good) and eventually comment on the approach? Did you have better ideas?

	Marco Bonardo [:mak] Assignee
	Comment 27 • 15 years ago

trunk unbitrot due to recent landings. branch will need a slightly different patch (no XPCOMUtils LazyGetters)

	Dan Mills [:thunder]
	Comment 28 • 15 years ago

How about having the customizer notify when it's done, and listen to that to dispose of it?

	Dan Mills [:thunder]
	Comment 29 • 15 years ago

Comment on attachment 403473 [details] [diff] [review]
patch v2.4

Up to you if you want to try the notification approach (see my last comment).  I think it might be cleaner, but have no great objection to this one, so r+.

	Marco Bonardo [:mak] Assignee
	Comment 30 • 15 years ago

yeah i thought about that one, but then each return point in the migration code should check if we have finished... btw i could just return checkCustomizationComplete(); instead of return nothing. I'll probably do that to avoid code duplication.

	Marco Bonardo [:mak] Assignee
	Comment 31 • 15 years ago

Using a "complete" notification, based on previous Thunder's comments.

	Marco Bonardo [:mak] Assignee
	Comment 32 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/4eaf2335e499

i'll soon attach patch for 1.9.1 branch.

	Kev Needham [:kev]
	Comment 33 • 15 years ago

Thanks, Marco. Really appreciate it.

	Marco Bonardo [:mak] Assignee
	Comment 34 • 15 years ago

bah, i was unsure where to remove the observer and in the last-minute i forgot to... additional changeset:
http://hg.mozilla.org/mozilla-central/rev/c5f3d2da6d52

i'm seeing a strange leak on 1.9.1 that is not present on m-c, investigating it.

	Marco Bonardo [:mak] Assignee
	Comment 35 • 15 years ago

so we leak due to the iniParser->getSections() call, and that is due to usage of nsCStringArray, on mozilla-central instead we use nsTArray.
that change was done in bug 466622, that we clearly can't take on 1.9.1.
I'll see if i can workaround this, otherwise there isn't much i can do about it.

	Marco Bonardo [:mak] Assignee
	Comment 36 • 15 years ago

So, i can't do anything about that leak, it's just exposed by the test, that code was not tested before, and is fixed on trunk.  Probably is not even worth filing a bug since it's fixed.

	Marco Bonardo [:mak] Assignee
	Comment 37 • 15 years ago

Comment on attachment 404648 [details] [diff] [review]
patch for 1.9.1

asking explicit approval to land on 1.9.1.
this has a test for a major part of sitribution that did not exist before.

	Marco Bonardo [:mak] Assignee
	Comment 38 • 15 years ago

s/sistribution/distribution/ clearly.

	Marco Bonardo [:mak] Assignee
	Comment 39 • 15 years ago

an i really think we also want this fixed on 1.9.2

	Marco Bonardo [:mak] Assignee
	Comment 40 • 15 years ago

pushed additional changeset to cleanup after a test failure (in case of)
http://hg.mozilla.org/mozilla-central/rev/e72c52753148

otherwise all next tests will import the distribution file.

	Jia Mi [:mijia]
	Comment 41 • 15 years ago

I still can reproduce this in the custom builds based on 3.5.4.
Is this supposed to be working in Firefox 3.5.4?

	Marco Bonardo [:mak] Assignee
	Comment 42 • 15 years ago

(In reply to comment #41)
> I still can reproduce this in the custom builds based on 3.5.4.
> Is this supposed to be working in Firefox 3.5.4?

no, it has not yet been approved.

	Marco Bonardo [:mak] Assignee
	Comment 43 • 15 years ago

asking blocking 3.6 too, we should fix it before the release.

	Marco Bonardo [:mak] Assignee
	Comment 44 • 15 years ago

Comment on attachment 404664 [details] [diff] [review]
patch for 1.9.2 branch

1.9.2 blocker, no more in need of approval.

	Marco Bonardo [:mak] Assignee
	Comment 45 • 15 years ago

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/34ed7e4a63f0

	Daniel Veditz [:dveditz]
	Comment 46 • 15 years ago

Comment on attachment 404648 [details] [diff] [review]
patch for 1.9.1

Approved for 1.9.1.6, a=dveditz for release-drivers

	Marco Bonardo [:mak] Assignee
	Comment 47 • 15 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d96a7f2709d6
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2b98553f1ed5