Closed Bug 516444 Opened 12 years ago Closed 12 years ago

Installation of Firefox Custom Builds without migration from a 2nd Browser is missing the Firefox default bookmarks

Categories

(Firefox :: Bookmarks & History, defect, P2)

3.5 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta2-fixed
status1.9.1 --- .6-fixed

People

(Reporter: cbook, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [has patch])

Attachments

(4 files, 6 obsolete files)

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.
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 ?
Keywords: regression
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
Summary: Installation of Firefox Custom Builds without migration from a 2nd is missing the Firefox default bookmarks → Installation of Firefox Custom Builds without migration from a 2nd Browser is missing the Firefox default bookmarks
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.
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.
Component: Release Engineering: Custom Builds → Bookmarks & History
Product: mozilla.org → Firefox
QA Contact: custom-builds → bookmarks
Target Milestone: --- → Firefox 3.5
Version: other → 3.5 Branch
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.
the above bug could be the unwilling culprit, by the fact it changes the order of Places initialization.
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.
where are handled distribution.ini contents?
Marco: modules/distribution.js
(around line 250, iirc)
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.
(In reply to comment #12)
> Btw i'm pretty sure this runs before browserGlue

i mean before Places initialization in browserGlue
blocking1.9.1: --- → ?
taking for now, since i know that code.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
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.
blocking1.9.1: ? → needed
Attached patch patch v1.0 (obsolete) — Splinter Review
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.
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.
Attachment #401524 - Attachment is obsolete: true
Attached patch patch v2.0 (obsolete) — Splinter Review
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.
Attachment #401631 - Attachment is patch: true
Attachment #401631 - Attachment mime type: application/octet-stream → text/plain
Flags: in-testsuite?
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).
(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.
Attached patch patch v2.1 (obsolete) — Splinter Review
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.
Attachment #401631 - Attachment is obsolete: true
Attachment #401743 - Flags: review?(thunder)
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.
Whiteboard: [has patch][needs review thunder]
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.
Attachment #401743 - Flags: review?(thunder) → review+
Attached patch patch v2.2 (obsolete) — Splinter Review
fix a typo, delete the customizer after applyBookmarks
Attachment #401743 - Attachment is obsolete: true
Whiteboard: [has patch][needs review thunder] → [has patch]
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.
Attachment #403226 - Attachment is obsolete: true
Attached patch patch v2.3 (obsolete) — Splinter Review
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?
Attachment #403245 - Flags: review?(thunder)
Attached patch patch v2.4 (obsolete) — Splinter Review
trunk unbitrot due to recent landings. branch will need a slightly different patch (no XPCOMUtils LazyGetters)
Attachment #403245 - Attachment is obsolete: true
Attachment #403473 - Flags: review?(thunder)
Attachment #403245 - Flags: review?(thunder)
How about having the customizer notify when it's done, and listen to that to dispose of it?
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+.
Attachment #403473 - Flags: review?(thunder) → review+
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.
Attached patch patch v2.5Splinter Review
Using a "complete" notification, based on previous Thunder's comments.
Attachment #403473 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/4eaf2335e499

i'll soon attach patch for 1.9.1 branch.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 3.5 → Firefox 3.7a1
Thanks, Marco. Really appreciate it.
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.
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.
Attached patch patch for 1.9.1Splinter Review
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.
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.
Attachment #404648 - Flags: approval1.9.1.5?
s/sistribution/distribution/ clearly.
an i really think we also want this fixed on 1.9.2
Attachment #404664 - Flags: approval1.9.2?
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.
blocking1.9.1: needed → ---
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?
(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.
asking blocking 3.6 too, we should fix it before the release.
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Priority: -- → P2
Attachment #404664 - Flags: approval1.9.2?
Comment on attachment 404664 [details] [diff] [review]
patch for 1.9.2 branch

1.9.2 blocker, no more in need of approval.
Comment on attachment 404648 [details] [diff] [review]
patch for 1.9.1

Approved for 1.9.1.6, a=dveditz for release-drivers
Attachment #404648 - Flags: approval1.9.1.6? → approval1.9.1.6+
You need to log in before you can comment on or make changes to this bug.