517515 - component registration and fastload invalidation triggers are not explicit

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Dietrich Ayala (:dietrich)]

in bug 511761 (and others), there's confusion about the implicit behaviors and side-effects of creating/update/deleting .autoreg, compreg.dat, and friends.

we should stop touching these files directly, and instead provide methods that achieve the end-goal explicitly. i don't know exactly what our needs are, but some examples:

registerAllComponents()
registerExtensionComponents()
invalidateAllFastloadCaches()
invalidateJSFastloadCache()
invalidateXULFastloadCache()

etc.

EIBTI!

Comment 1

[Benjamin Smedberg]

I can't think of a reason we'd need to invalidate the various fastload caches separately: one method to invalidate all of them should be fine.

This should hang off nsIXULRuntime if it needs to be scriptable.

Comment 2

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

not sure if bug 364864 would be affected by this or other bugs being worked on to change our comp reg invalidation but pointing it out just in case.

Comment 3

[Benedict Hsieh [:benedict]]

Attachment: provides an interface for "invalidate all caches on restart"

requires patch in bug 511761

Adds a (scriptable) method to nsIXULRuntime that allows callers to signal for the caches to be invalidated on the next restart. When called, the method writes a flag to compatibility.ini. On application start, we check for that flag and if we find it we invalidate the caches and remove the flag.

Currently the only caller is the extensions manager.

Comment 4

[(dormant account)]

Comment on attachment 403653 [details] [diff] [review]
provides an interface for "invalidate all caches on restart"

>+  nsCOMPtr<nsIFile> file;
>+  profDir->Clone(getter_AddRefs(file));
>+  if (!file)
>+    return NS_ERROR_NOT_AVAILABLE;
No need for clone here. profDir is a brand new nsifile object, just name it file.

>+  if (NS_FAILED(rv)) {
>+    localFile->OpenNSPRFileDesc(PR_RDWR | PR_APPEND, 0600, &fd);
>+    if (!fd) {
>+      NS_ERROR("could not create output stream");
>+      return NS_ERROR_NOT_AVAILABLE;
>+    }
>+    static const char kInvalidationHeader[] = NS_LINEBREAK "InvalidateCaches=1" NS_LINEBREAK;
>+    PR_Write(fd, kInvalidationHeader, sizeof(kInvalidationHeader) - 1);
need to check/propagate error codes on above.

>+    PR_Close(fd);
>+  }
>+  return NS_OK;
>+}
>+
> #ifdef XP_WIN
> // Matches the enum in WinNT.h for the Vista SDK but renamed so that we can
> // safely build with the Vista SDK and without it.
> typedef enum 
> {
>   VistaTokenElevationTypeDefault = 1,
>   VistaTokenElevationTypeFull,
>   VistaTokenElevationTypeLimited
>@@ -2142,75 +2183,79 @@ SelectProfile(nsIProfileLock* *aResult, 
>       return ProfileLockedDialog(profileDir, profileLocalDir, unlocker,
>                                  aNative, aResult);
>     }
>   }
> 
>   return ShowProfileManager(profileSvc, aNative);
> }
> 
>-#define FILE_COMPATIBILITY_INFO NS_LITERAL_CSTRING("compatibility.ini")
>-
>-static PRBool
>+static void
> CheckCompatibility(nsIFile* aProfileDir, const nsCString& aVersion,
>                    const nsCString& aOSABI, nsIFile* aXULRunnerDir,
>-                   nsIFile* aAppDir)
>+                   nsIFile* aAppDir, PRBool* aVersionOK, PRBool* aCachesOK)
> {

This is weird. Use that return value instead of moving stuff into outparams. 

Add a comment briefly describing what the function does and return value. Also can aVersionOK/aCachesOk not be collapsed into one? Why not collapse those into nsresult, where NS_OK means everything is alright.

As a rule we try to not set outparams when an error occurs. Use local variables and then set outparams IFF returning NS_OK.

> 
> static void BuildVersion(nsCString &aBuf)
> {
>   aBuf.Assign(gAppData->version);
>   aBuf.Append('_');
>   aBuf.Append(gAppData->buildID);
>   aBuf.Append('/');
>@@ -2266,31 +2311,16 @@ WriteVersion(nsIFile* aProfileDir, const
>   }
> 
>   static const char kNL[] = NS_LINEBREAK;
>   PR_Write(fd, kNL, sizeof(kNL) - 1);
> 
>   PR_Close(fd);
> }
> 
>-static PRBool ComponentsListChanged(nsIFile* aProfileDir)
>-{
>-  nsCOMPtr<nsIFile> file;
>-  aProfileDir->Clone(getter_AddRefs(file));
>-  if (!file)
>-    return PR_TRUE;
>-  file->AppendNative(NS_LITERAL_CSTRING("compreg.dat"));
>-
>-  PRBool exists = PR_FALSE;
>-  file->Exists(&exists);
>-  
>-  // If we need a re-register, someone should have removed this file.
>-  return !exists;
>-}
>-
> static void RemoveComponentRegistries(nsIFile* aProfileDir, nsIFile* aLocalProfileDir,
>                                       PRBool aRemoveEMFiles)
> {
>   nsCOMPtr<nsIFile> file;
>   aProfileDir->Clone(getter_AddRefs(file));
>   if (!file)
>     return;
> 
>@@ -3167,38 +3197,44 @@ XRE_main(int argc, char* argv[], const n
> #else
>     // No TARGET_XPCOM_ABI, but at least the OS is known
>     NS_NAMED_LITERAL_CSTRING(osABI, OS_TARGET "_UNKNOWN");
> #endif
> 
>     // Check for version compatibility with the last version of the app this 
>     // profile was started with.  The format of the version stamp is defined
>     // by the BuildVersion function.
>-    PRBool versionOK = CheckCompatibility(profD, version, osABI,
>-                                          dirProvider.GetGREDir(),
>-                                          gAppData->directory);
>+    // Also check to see if something has happened to invalidate our
>+    // fastload caches, like an extension upgrade or installation.
>+    PRBool versionOK, cachesOK;
>+    CheckCompatibility(profD, version, osABI, dirProvider.GetGREDir(),
>+                       gAppData->directory, &versionOK, &cachesOK);
> 
>     // Every time a profile is loaded by a build with a different version,
>     // it updates the compatibility.ini file saying what version last wrote
>     // the compreg.dat.  On subsequent launches if the version matches, 
>     // there is no need for re-registration.  If the user loads the same
>     // profile in different builds the component registry must be
>     // re-generated to prevent mysterious component loading failures.
>     //
>     if (gSafeMode) {
>       RemoveComponentRegistries(profD, profLD, PR_FALSE);
>       WriteVersion(profD, NS_LITERAL_CSTRING("Safe Mode"), osABI,
>                    dirProvider.GetGREDir(), gAppData->directory);
>     }
>     else if (versionOK) {
>-      if (ComponentsListChanged(profD)) {
>+      if (!cachesOK) {
>         // Remove compreg.dat and xpti.dat, forcing component re-registration.
>         // The new list of additional components directories is derived from
>         // information in "extensions.ini".
>         RemoveComponentRegistries(profD, profLD, PR_FALSE);
>+        
>+        // Rewrite compatibility.ini to remove the flag
>+        WriteVersion(profD, version, osABI,
>+                     dirProvider.GetGREDir(), gAppData->directory);
>       }
>       // Nothing need be done for the normal startup case.
>     }
>     else {
>       // Remove compreg.dat and xpti.dat, forcing component re-registration
>       // with the default set of components (this disables any potentially
>       // troublesome incompatible XPCOM components). 
>       RemoveComponentRegistries(profD, profLD, PR_TRUE);
>diff --git a/xpcom/system/nsIXULRuntime.idl b/xpcom/system/nsIXULRuntime.idl
>--- a/xpcom/system/nsIXULRuntime.idl
>+++ b/xpcom/system/nsIXULRuntime.idl
>@@ -81,9 +81,16 @@ interface nsIXULRuntime : nsISupports
>    */
>   readonly attribute AUTF8String XPCOMABI;
> 
>   /**
>    * A string tag identifying the target widget toolkit in use.
>    * This is taken from the MOZ_WIDGET_TOOLKIT configure variable.
>    */
>   readonly attribute AUTF8String widgetToolkit;
>+
>+  /**
>+   * Signal the apprunner to invalidate caches on the next restart.
>+   * This will cause components to be autoregistered and all
>+   * fastload data to be re-created.
>+   */
>+  void invalidateCachesOnRestart();

Need to generate new uuid when changing idl files.
https://developer.mozilla.org/en/Generating_GUIDs

> };

Also, it may make sense to fold bug 511761 into this and get them review at the same time. It's your choice

Comment 5

[Benedict Hsieh [:benedict]]

This should be resolved with the patch in bug 511761. The way for application code to invalidate caches is to call nsIXULRuntime::InvalidateCachesOnRestart(). Caches can also be invalidated by changes to the buildID or other things listed in nsAppRunner::CheckCompatibility, but this happens through the build system or other non-application-code things.

All of these methods use compatibility.ini, incidentally.