299859 - Firefox reregisters all modules 3 times on every startup

Status: RESOLVED WORKSFORME

(Toolkit :: Startup and Profile System, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: MOZ_CO_DATE="Fri Jul 1 23:18:40 CDT 2005" debug build

STEPS TO REPRODUCE: start the build (with -safe-mode if desired; no effect from
doing that).

ACTUAL RESULTS:  "No persistent registry found", followed by registering all
modules three times.  In the process, we leak all sorts of stuff, including
nsDocument objects.

NOTE: other builds pulled at the same time (on a different machine) do not show
this problem.

Comment 1

[Benjamin Smedberg]

bz, could I get you attach the following files (singly or zipped):

<profile>/extensions.rdf
<profile>/extensions-startup.manifest
<profile>/extensions.ini
<profile>/extensions/* (if there are any profile-installed extensions at all)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: zip of profile

Comment 3

[Benjamin Smedberg]

This is interesting, but I bet the steps to reproduce are going to be a royal
pain if it can be reproduced at all: the extensions-startup.manifest file bz
attached has two separate entries for the default theme:

app-global	{972ce4c6-7e08-4474-a285-3208198ce6fd}
/home/bzbarsky/mozilla/debug/obj-firefox/dist/bin/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}
1115643426	needs-install
app-profile	{972ce4c6-7e08-4474-a285-3208198ce6fd}
extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}	1114971805	

The upper one is "incorrect" in that we should have ignored it when we first
upgraded from the 1.0 -> 1.1 profile, and now it's stuck in an everlasting
needs-restart loop, saved only by the last-ditch recursion check in
nsAppStartup.cpp that prevents us from eternal restarting.

Comment 4

[Robert Strong (they/them - no direct email)]

The app-profile entry for the default theme is most likely be due to having a
profile that was upgraded prior to the 1.0 profile upgrade code that landed in
bug 295855. Since having a default theme located in app-profile is a bad thing
for several reasons perhaps it should be checked for existence and removed if
found during every profile upgrade (e.g. 1.0+ to 1.1, 1.1 to 1.0, etc.) and not
just during the conditions for the 1.0 profile upgrade (e.g. existence of an
Extensions.rdf in the extensions dir).

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: safegaurd during upgrade

I suspect this bug will only affect those that upgraded prior to the checkin of
bug 295855. Checking for the existence of a duplicate theme dir when a app
version mismatch is detected seems like a better way to safe gaurd against this
without a significant perf impact. Another options would be to check for an
app-profile theme being installed during every startup and to check during the
install of any item that it isn't using the default theme id.

My statement of "existence of an Extensions.rdf in the extensions dir" should
have included without an extensions.rdf in the profile dir.

Comment 6

[Benjamin Smedberg]

Comment on attachment 188464 [details] [diff] [review]
safegaurd during upgrade

I'm not sure what I think of this: will this break people who switch back and
forth from 1.0 <-> 1.1?

Comment 7

[Robert Strong (they/them - no direct email)]

When I tested it when writing the patch in bug 295855 it didn't prevent
downgrading to 1.0 since the reading of the install.rdf only happens once in 1.0
for the default theme... from that point onward it used the entry in
extensions/Extensions.rdf without caring one way or the other about the
existence of the directory. Also, without this patch the same conditions will
occur in regards to the directory not existing when upgrading from 1.0 to a
current nightly, etc. This just removes the directory under more conditions.

I believe this bug is an edge case that will only affect nightly users that
upgraded during a specific time frame. The only reason I think this approach
should be considered is that there may be other cases, the code to fix this is
simple with no known harmful side affects found through testing, and having an
app-profile default theme breaks other things besides multiple registration
(e.g. unable to use the default theme if you upgrade with a non default theme in
use).

Comment 8

[Robert Strong (they/them - no direct email)]

I don't think this patch is going to be necessary. This problem only affects a
small number of trunk users and a new profile fixes this.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm sorry, but "create a new profile" is never the right answer...

Comment 10

[Robert Strong (they/them - no direct email)]

(In reply to comment #9)
> I'm sorry, but "create a new profile" is never the right answer...
Just weighing the risk / benefit ratio in relation to the number of reports of
people experiencing this problem. For example, the checks could be added to
every startup but that would of course cause a perf impact which isn't the right
answer either. In this specific instance the "right answer" IMO is to not put
ourselves in a situation like this when it can be avoided which in this case it
could have been. Also, the patch I came up with will only fix this during a
version upgrade so it isn't an immediate fix. For reference, the bug that
introduced this is bug 286034 and the followup for the regression is bug 295855.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So if this is basically an instance of profile corruption by nightlies, I can
understand the "new profile" approach.  If this is something that would bite
pretty much any nightly tester, then I think it's worth addressing...

Comment 12

[Robert Strong (they/them - no direct email)]

Agreed... this only affects a subset of nightly testers and only this subset
would need to create a new profile.

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 188464 [details] [diff] [review]
safegaurd during upgrade

Not checked in - not needed.

Comment 14

[Robert Strong (they/them - no direct email)]

After waiting several months and watching for reports where this bug is experienced it appears that comment #8 is correct in the assumption that this is an edgecase that only affected a small number of trunk users. Resolving -> WFM.