304115 - compreg.dat isn't rebuilt properly after Software Update

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[shutdown]

After doing Software Update, compreg.dat is rebuilt with entries for
the components contained in the extensions (e.g. inspector.dll) missing.
related to: bug 297312.

Program flow after Software Update:

0. The application is started by updater.exe (the update has been applied).
1. Mismatch of the profile version is detected by the file "compatibility.ini".
  XRE_main() => CheckCompatibility()
  http://lxr.mozilla.org/seamonkey/source/toolkit/xre/nsAppRunner.cpp#2101
2. The files "compreg.dat", "xpti.dat", ".autoreg", "extensions.ini",
   and "XUL.mfl" stored in the profile directory are removed.
  XRE_main() => RemoveComponentRegistries()
  http://lxr.mozilla.org/seamonkey/source/toolkit/xre/nsAppRunner.cpp#2131
3. The file "compatibility.ini" is rebuilt.
  XRE_main() => WriteVersion()
  http://lxr.mozilla.org/seamonkey/source/toolkit/xre/nsAppRunner.cpp#2141
4. The files "compreg.dat" and "xpti.dat" are rebuilt.
   However, the components contained in the extensions are not registered
   because there is no extensions.ini and XPCOM can't detect any extensions.
  XRE_main() => ScopedXPCOMStartup::Initialize() => NS_InitXPCOM3()
5. Extension Manager rebuilds the file "extensions.ini".
  XRE_main() => ExtensionManager,prototype.start()
  http://lxr.mozilla.org/seamonkey/source/toolkit/xre/nsAppRunner.cpp#2241
6. The browser is restarted.
  http://lxr.mozilla.org/seamonkey/source/toolkit/xre/nsAppRunner.cpp#2352
7. This time, the profile version matches and no files are rebuilt.
   There is no chance to register the components contained in the extensions.

Solution:

Rebuild compreg.dat and xpti.dat after extensions.ini is rebuilt.

Comment 1

[Benjamin Smedberg]

ick

Comment 2

[Darin Fisher]

I think this is a pretty serious bug.  Rob, can you take a look.  We are
probably not writing out .autoreg properly after writing extensions.ini.  I
believe this bug can happen outside the context of software update.

-> rstrong

Comment 3

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

I'll take a look at it this evening... strange I haven't seen this when doing an
upgrade or running different versions of the app from different locations.
Overall, the startup code for the EM needs some love but not for 1.8

Comment 4

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

Attachment: patch

>     var componentList = getFile(KEY_PROFILEDIR, [FILE_EXTENSION_MANIFEST]);
>     if (!componentList.exists())
>-	isDirty = true;
>+	var forceAutoReg = true;
This is safe to use because immediately following this the call to
_ensureDatasetIntegrity sets isDirty to true if this same file doesn't exist.
     
>     // Extension Changes
>-    if (isDirty)
>-	return this._finishOperations();
>+    if (isDirty) {
>+	var needsRestart = this._finishOperations();
>+
>+	if (forceAutoReg) {
>+	  this._extensionListChanged = true;
>+	  return true;
>+	}
>+	return needsRestart;
>+    }
_finishOperations won't always return true or create an .autoreg file if there
is no extensions.ini so we always create a .autoreg file and return true if the
extensions.ini doesn't exist... otherwise we return the value from
_finishOperations.

>     gPref.setCharPref(PREF_EM_LAST_APP_VERSION, currAppVersion);
>+    this._updateManifests(true);
>     return true;
In some cases we can end up with an incomplete extensions.ini on upgrade which
this fixes.

Another item of interest - can't remove a file if it doesn't exist. I am
hesitant to fix due to not knowing the ramifications it may have. 
http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#3400


I'd prefer to re-write and combine checkForMismatches and start as we discussed
on irc but I doubt that would be appropriate for 1.8b4 ;-)

Comment 5

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

Attachment: added another fix

If there is no lastAppVersion (e.g. new profile) don't perform a compatibility
check.

Comment 6

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

bug 304269 covers the issue at the end of comment #4

Comment 7

[Benjamin Smedberg]

Comment on attachment 192340 [details] [diff] [review]
added another fix

it's bad style to set a "var foo" inside an if block without setting it
otherwise. You should probably do "var forceAutoReg = false" at the top of the
function and set it to true in the if block. At least that would confuse me
less ;-)

Comment 8

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

Attachment: patch

Addresses comment and sets the last app version if it doesn't exist before
returning early in checkForMismatches... this is closer to the old behavior
where it used to set it every time checkForMismatches was called even if it was
the same.

Comment 9

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

Fixed for 1.8b4