293461 - Make Safe Mode a generic startup flag, not just for EM, and make its handling less fragile.

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect)

Description

[Benjamin Smedberg]

The current handling of -safe-mode is done entirely by the extension manager and
it is rather fragile. We should just have safe mode handled by the nsAppRunner
startup code and add a flag to nsIXULAppInfo or a similar interface.

Comment 1

[Benjamin Smedberg]

Attachment: Make safe mode a generic startup flag, rev. 1

This patch does several different things:

1) it prepares nsXREAppData and nsIXULAppInfo for freezing
2) Safe mode flags are read very early in app startup: the xre dir provider
knows about safe mode and does not read any extension manifests when safe mode
is enabled
3) This reduces a lot of complexity from the EM, which doesn't have to keep
track of a lot of safe mode garbage any more
4) Also implement what I planned in bug 275529 - allow and encourage appID and
extensionID to be a prettyname (formatted like an email) instead of a {GUID}.

Comment 2

[Darin Fisher]

Comment on attachment 183178 [details] [diff] [review]
Make safe mode a generic startup flag, rev. 1

>Index: modules/libpref/src/nsPrefBranch.cpp

>-  NS_INTERFACE_MAP_ENTRY(nsIPrefBranch2)
>-  NS_INTERFACE_MAP_ENTRY(nsIPrefBranchInternal)
>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIPrefBranch2, !mIsDefault)
>+  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIPrefBranchInternal, !mIsDefault)

please verify that this does not break anybody who might be getting
the default branch and trying to observer user pref changes.  LXR
search and verify :)


>Index: toolkit/xre/nsAppRunner.cpp

> NS_IMETHODIMP
> nsXULAppInfo::GetVendor(nsACString& aResult)
> {
>-  aResult.Assign(gAppData->appVendor ? gAppData->appVendor : "");
>+  aResult.Assign(gAppData->vendor);

this is abusing the string API.  the string code only tolerates
this particular use case of passing null to work around bad callers.
eventually, the plan was to fix callers so that it must not be null.
but, maybe that is fruitless and whatever... /sigh/... no need to
make any changes here.


>+#if defined(XP_WIN) || defined(XP_OS2)
>+#define FASTLOAD_FILENAME "XUL.mfl"
>+#else
>+#define FASTLOAD_FILENAME "XUL.mfasl"
>+#endif

I think you should expose a simple function from nsFastLoadService
that will generate this for you using the same algorithm.  That
would avoid having to keep these two bits of code in sync.  It will
be hard to notice if they ever get out of sync.

class nsFastLoadService {
  ...
public:
  static void MakeFilename(const char *aBaseName, nsACString &aResult);
  ...
};


>+  file->SetNativeLeafName(NS_LITERAL_CSTRING(FASTLOAD_FILENAME));
>+  file->Remove(PR_FALSE);

What about the fastload file that lives in the "local" profile dir?
Why aren't you deleting that one?  The one you are deleting will
shortly never exist.

Are there specific problems you are trying to solve by deleting the
fastload file?	Or, are you just trying to play it safe?  I think 
this is probably a good idea for that reason, but I'm just curious
if you have any evidence of actual bugs that this would fix.  Is
this about sharing profiles across different UNIX systems (as was
discussed on IRC)?



>Index: toolkit/xre/nsIXULAppInfo.idl

>   /**
>-   * The name of the application vendor. This is ASCII, and is normally
>-   * mixed-case; e.g. "Mozilla". It may not be specified, resulting in the
>-   * empty string.
>+   * @returns an empty string if nsXREAppData.vendor is not set.
>    */
>   readonly attribute ACString vendor;
> 
>-  /**
>-   * The name of the application. This is ASCII, and is normally mixed-case;
>-   * e.g. "Firefox".
>-   */
>   readonly attribute ACString name;

why are you deleting these comments?  please add back some comments.
if this interface is to be frozen, it needs comments yadda yadda...

are you trying to make it so that people should look at nsXREAppInfo
for documentation?


>Index: toolkit/xre/nsXULAppAPI.h

>+  /**
>    * The name of the application vendor. This must be ASCII, and is normally
>+   * mixed-case, e.g. "Mozilla". Optional, but highly recommended.
>    */
>+  const char *vendor;

should define what "optional" means... it means "may be null"


> 
>   /**
>    * The name of the application. This must be ASCII, and is normally
>    * mixed-case, e.g. "Firefox".
>    */
>-  const char *appName;
>+  const char *name;

"This must be non-empty and non-null"


>   /**
>-   * The major version, e.g. "0.8.0+"
>+   * The major version, e.g. "0.8.0+". Optional, but required for
>+   * advanced application features such as the extension manager and
>+   * update service.
>    */
>+  const char *version;

again, define optional.


>  * @note           If the binary is linked against the  standalone XPCOM glue,
>  *                 XPCOMGlueStartup() should be called before this method.
>  *
>  * @note           XXXbsmedberg Nobody uses the glue yet, but there is a
>+ *                 potential problem: on windows, the glue calls
>  *                 SetCurrentDirectory, and relative paths on the command line
>  *                 won't be correct.

ack.. this comment applies to the "full" glue library, not the "smaller" glue
library.  worth it to distinguish here?


>Index: toolkit/mozapps/extensions/content/extensions.js

>+  gApp = Components.classes["@mozilla.org/xre/app-info;1"].getService(Components.interfaces.nsIXULAppInfo)
>+                   .QueryInterface(Components.interfaces.nsIXULRuntime);

This looks odd, but i get it.  you're making gApp have properties from
both interfaces.  maybe fix this guy to wrap at 80 chars or somewhere
close to that?


>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in

>+var gGUIDTest = /(\{[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\}|[a-z0-9-\._]*\@[a-z0-9-\._\{\}]+)\/?$/i;

calling this "gGUIDTest" seems way wrong now.  how about gIDTest instead?


>+function inSafeMode() {
>+  return gApp.QueryInterface(Components.interfaces.nsIXULRuntime).inSafeMode;
>+}

this QI is no longer needed right?


>Index: xulrunner/app/nsXULRunnerApp.cpp

>+  nsXREAppData appData = {
>+    sizeof(nsXREAppData),
>+    nsnull,
>+    nsnull,
>+    nsnull,
>+    nsnull,
>+    nsnull,
>+    nsnull,
>+    nsnull,
>+    nsnull,
>+    nsnull
>+  };

FWIW, you should be able to just write this like so:

    nsXREAppData appData = { sizeof(nsXREAppData), 0 };

The "0" will be repeated by the compiler to the end of the structure.
This is the cheap way of zero'ing out a data structure.


What about calendar?

Comment 3

[Darin Fisher]

By the way, I really like this patch.  r=me with the issues I pointed out addressed.

Comment 4

[Jens Bannmann]

Comment on attachment 183178 [details] [diff] [review]
Make safe mode a generic startup flag, rev. 1

> /** 
>  * Valid GUIDs fit this pattern.
>  */
>-var gGUIDTest = /(\{[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\})\/?$/i;
>+var gGUIDTest = /(\{[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\}|[a-z0-9-\._]*\@[a-z0-9-\._\{\}]+)\/?$/i;

Umm... shouldn't you include a ^ marker in the regex, so
"foobar{a4914387-e151-4c8a-b169-7cc022e0a3fd}" doesn't match the pattern?

Comment 5

[Benjamin Smedberg]

Attachment: Make help content an integral part of the build process, rev. 1.1

Nits fixed. Used a #define in nsIFastLoadService.idl instead of a function.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 183194 [details] [diff] [review]
Make help content an integral part of the build process, rev. 1.1

+   * @see nsXREAppData.vendor
+   * @returns an empty string if nsXREAppData.vendor is not set.

hm, is it nice to refer from documentation of public scriptable APIs to
C++-only structs?

toolkit/mozapps/extensions/public/nsIExtensionManager.idl needs a new IID

Comment 7

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

Comment on attachment 183194 [details] [diff] [review]
Make help content an integral part of the build process, rev. 1.1

-var gGUIDTest =
/(\{[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\})\/?$/i;
+var gIDTest =
/^(\{[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\}|[a-z0-9-\._
]*\@[a-z0-9-\._\{\}]+)\/?$/i;
Nit: Is there a reason for allowing { or } in the vendor.tld section? If not, I
think it would be clearer to remove it and go with a comment of something like
this since it is just masquerading as a tld
* a more readable form is encouraged: "appname@vendor" where 
* appname and vendor must contain one or more of the characters
* a-z A-Z 0-9 - . _ and must be seperated by a single @ character.

>     case "cmd_options":
>       return selectedItem &&
>              !selectedItem.disabled &&
>+             !gApp.inSafeMode &&
>              selectedItem.getAttribute("toBeUninstalled") != "true" &&
>              selectedItem.getAttribute("optionsURL") != "";
The value of selectedItem.disabled already returns true from _rdfGet_disabled
in nsExtensionManager.js when in safe mode.

>     case "cmd_uninstall":
>       if (gWindowState != "extensions") {
>         // uninstall is only available if the selected item isn't the 
>         // default theme.
>+        
>+
Nit: some extra lines snuck in

>         return (selectedItem && 
>                 selectedItem.getAttribute("internalName") != KEY_DEFAULT_THEME);
>       }
This constant was removed and replaced with gDefaultTheme

>     case "cmd_reallyEnable":
>     // controls whether to show Enable or Disable in extensions' context menu
>       return selectedItem && 
>-             selectedItem.disabled && 
>-             !gExtensionManager.inSafeMode;
>+             selectedItem.disabled;
>     case "cmd_enable":
>     //controls wheter the Enable/Disable menuitem is enabled
>       return selectedItem && 
>
>              selectedItem.disabled && 
>-             !gExtensionManager.inSafeMode && 
>              selectedItem.getAttribute("toBeUninstalled") != "true" &&
>              selectedItem.getAttribute("compatible") != "false";
>     case "cmd_disable":
>       return selectedItem &&
>              !selectedItem.disabled &&
>              selectedItem.getAttribute("toBeUninstalled") != "true" && 
>              selectedItem.getAttribute("toBeInstalled") != "true";
This won't set the context menu properly when in safe mode. One example is the
cmd_enable case for an enabled extension when in safe mode... since
_rdfGet_disabled in nsExtensionManager.js will always return true when in safe
mode.

Comment 8

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

One more item in nsExtensionManager.js:
-    if (this.safeMode && !item.EqualsNode(this._defaultTheme)) 
+    if (inSafeMode())
This will prevent selecting use theme for the default theme when in safe mode.

Comment 9

[Benjamin Smedberg]

Attachment: Make help content an integral part of the build process, rev. 1.2

ok, I got rid of the "disabled" bit, so that you can disable/enable any
extension/theme item from safe mode. When adding the ^ to gIDTest I discovered
that EM sometimes calls it with full paths instead of just leafnames, so I
fixed that as well.

This patch is sometimes triggering bug 280234 (safe-mode only, when opening the
extension manager). I am investigating that separately; this should be ready
for review independent of that problem.

Comment 10

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

The same logic for disabled with the cmd_about case is needed for safe mode with
the cmd_about case. Something like              
(gApp.inSafeMode ? selectedItem.getAttribute("aboutURL") == "" : true);

It would be better to show the default about dialog when in safe mode and for
disabled extensions but that is outside the scope of this.

Great patch and with that issue addressed r=me

Comment 11

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

After looking at how to resolve this it would probable be easier to add the
following to nsExtensionManager.js.in around line 5371

  /**
   * Get the em:aboutURL property (about url of the item)
   * returns an empty string when in safe mode
   */
  _rdfGet_aboutURL: function(item, property) {
    if (inSafeMode())
      return EM_L("");
    return null;
  },

Comment 12

[Benjamin Smedberg]

Attachment: Make help content an integral part of the build process, rev. 1.3

That's great.

Comment 13

[Asa Dotzler [:asa]]

Comment on attachment 183309 [details] [diff] [review]
Make help content an integral part of the build process, rev. 1.3

a=asa

Comment 14

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

Comment on attachment 183309 [details] [diff] [review]
Make help content an integral part of the build process, rev. 1.3

Looks good.

Comment 15

[Benjamin Smedberg]

Fixed on trunk.

Comment 16

[David G King]

For reasons that I don't fully understand, and thus may have the wrong bug, but
it seems that this bug has caused a problem as follows :-

e:/mozilla/source/mozilla/calendar/sunbird/app/nsCalendarApp.cpp:57: error: brac
e-enclosed initializer used to initialize `const char*'
e:/mozilla/source/mozilla/calendar/sunbird/app/nsCalendarApp.cpp:57: error: inva
lid conversion from `const char*' to `PRUint32'
e:/mozilla/source/mozilla/calendar/sunbird/app/nsCalendarApp.cpp:57: error: cann
ot convert `const char*' to `nsILocalFile*' in initialization
e:/mozilla/source/mozilla/calendar/sunbird/app/nsCalendarApp.cpp:57: error: inva
lid conversion from `int' to `const char*'
make[4]: *** [nsCalendarApp.o] Error 1



Line 57 is part of the following, and I assume that
NS_XRE_ENABLE_EXTENSION_MANAGER is the problem part :-

48 static const nsXREAppData kAppData = {
49   "Mozilla",
50   "Sunbird",
51   NS_STRINGIFY(APP_VERSION),
52   NS_STRINGIFY(BUILD_ID),
53   // {718e30fb-e89b-41dd-9da7-e25a45638b28}
54   { 0x718e30fb, 0xe89b, 0x41dd, { 0x9d, 0xa7, 0xe2, 0x5a, 0x45, 0x63, 0x8b,
0x28 } },
55   "Copyright (c) 2004 mozilla.org",
56   NS_XRE_ENABLE_EXTENSION_MANAGER
57 };

Comment 17

[Darin Fisher]

The application GUID needs to be converted to a string value instead of a nsID
structure.

Comment 18

[David G King]

So, as the struct was changed here, should the repercussions also be fixed here,
or in a seperate bug?

Comment 19

[Benjamin Smedberg]

As the world of xul apps widens, I don't want to have to fix every one as the
API changes, especially since there are no Sunbird tinderboxes (hopefully I can
freeze the API soon and we won't have this problem). File a new bug with a
patch, or just have one of the Sunbird devs fix it.

Comment 20

[Jens Bannmann]

Another nit on the ID pattern:
var gIDTest = /^(GUIDFORMATHERE|[a-z0-9-\._]*\@[a-z0-9-\._]+)\/?$/i;

Wouldn't this allow something like "{a4914387-e151-4c8a-b169-7cc022e0a3fd}/"
(note the trailing slash)? It seems to me the slash would either belong inside
the ID-like part of the regex, or should be dropped.

Comment 21

[Benjamin Smedberg]

Jens, that extra slash is part of the regex because it is sometimes applied to
directory leafnames which have a trailing /. The capturing parens do not capture
the slash, so it's not part of RegEx.$1.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The checkin has fixed bug 289523.