149208 - NS_InitEmbedding unconditionally calls AutoRegister

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Brian Ryner (not reading)]

NS_InitEmbedding() calls AutoRegister() unconditionally.  This just seems wrong.
 We should not default to autoregistering on each run for release builds
(particularly since autoregistration is somewhat slow for static builds).

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Here's one way to fix the problem... the inline functions in nsISoftwareUpdate
provide a means to know that you've already autoregistered, as well as a way
for an embeddor to trigger re-registration.

Comment 2

[Brian Ryner (not reading)]

Attachment: diff -w version of the above patch

Comment 3

[Brian Ryner (not reading)]

per conversation with dougt, we don't want to introduce a dependency on
xpinstall here.  Instead, xpcom could maintain the pending autoreg state.  That
would also allow us to get rid of libreg on the trunk.

Comment 4

[Doug Turner (:dougt)]

Attachment: proposed patch v.1

Basically what this is is uses a file instead of the registry as a signal to
autoreg.

Comment 5

[Doug Turner (:dougt)]

Attachment: embedding patch

this makes embedding always autoreg only in debug.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 94654 [details] [diff] [review]
proposed patch v.1

>Index: xpinstall/public/nsISoftwareUpdate.h
>===================================================================
>RCS file: /cvsroot/mozilla/xpinstall/public/nsISoftwareUpdate.h,v
>retrieving revision 1.29
>diff -u -r1.29 nsISoftwareUpdate.h
>--- xpinstall/public/nsISoftwareUpdate.h	23 Mar 2002 12:14:32 -0000	1.29
>+++ xpinstall/public/nsISoftwareUpdate.h	9 Aug 2002 18:39:18 -0000
> /**
>  * Request that an autoreg be performed at next startup. (Used
>- * internally by XPI.)
>+ * internally by XPI.)  This basically drops a files next to the
>+ * application.  Next time XPCOM sees this file, it will cause
>+ * a autoreg, then delete this file.

nit: I think "an autoreg" is correct grammatically.

>+  nsCOMPtr<nsIFile> file;
>+  NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR,
>+                         getter_AddRefs(file));
>+  

CURRENT_PROCESS_DIR is something different from the current working directory,
right? It won't change during execution?

>+  rv = file->Create(nsIFile::NORMAL_FILE_TYPE, 0777);

Does it really need to be 777?

>-
>+    
>     // Pay the cost at startup time of starting this singleton.
>     nsIInterfaceInfoManager* iim = XPTI_GetInterfaceInfoManager();
>     NS_IF_RELEASE(iim);
>@@ -500,6 +537,7 @@
>     
>     return rv;
> }
>+
> 

MIght want to undo the unneeded whitespace changes, unless they're fixing bad
spacing.

>Index: xpfe/bootstrap/nsAppRunner.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/bootstrap/nsAppRunner.cpp,v
>retrieving revision 1.361
>diff -u -r1.361 nsAppRunner.cpp
>--- xpfe/bootstrap/nsAppRunner.cpp	7 Aug 2002 08:54:08 -0000	1.361
>+++ xpfe/bootstrap/nsAppRunner.cpp	9 Aug 2002 18:39:58 -0000
> #ifdef DEBUG
>   // _Always_ autoreg if we're in a debug build, under the assumption
>   // that people are busily modifying components and will be angry if
>   // their changes aren't noticed.
>-  needAutoReg = PR_TRUE;
>-#endif
>-
>-  if (needAutoReg) {
>     nsComponentManager::AutoRegister(nsIComponentManagerObsolete::NS_Startup,
>                                      NULL /* default */);

From here it looks like this line should be un-indented since the "if" block is
no longer there.

Comment 7

[Doug Turner (:dougt)]

nit fixed.
permission does not really matter, but I changed it to 666.
whitespace fixed.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 94654 [details] [diff] [review]
proposed patch v.1

sr=bryner

Comment 9

[Brian Ryner (not reading)]

Comment on attachment 94658 [details] [diff] [review]
embedding patch

sr=bryner

Comment 10

[Chak Nanga]

Comment on attachment 94658 [details] [diff] [review]
embedding patch

r=chak

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 94654 [details] [diff] [review]
proposed patch v.1

>Index: xpinstall/src/nsSoftwareUpdate.cpp
>===================================================================
> NS_IMETHODIMP
> nsSoftwareUpdate::StartupTasks( PRBool *needAutoreg )
> {
>+    return NS_ERROR_NOT_IMPLEMENTED;
> }

Please just delete this uncalled method and take it out of the .h file(s).
http://lxr.mozilla.org/seamonkey/ident?i=StartupTasks

>Index: xpcom/build/nsXPComInit.cpp
>===================================================================
> 
>+static PRBool CheckAndRemoveUpdateFile()
>+{
>+        return NS_ERROR_FAILURE;

This will be interpreted as "true". change to PR_FALSE.

>+    PRBool exists;
>+    file->Exists(&exists);
>+    if (!exists)
>+        return exists;

Can file->Exists fail? I've seen odd things when people ignore
nsILocalFile return codes. Please check it, or at the very least
set a default PR_FALSE value for exists so it's not read uninitialized.

>Index: xpfe/bootstrap/nsAppRunner.cpp
>===================================================================
>-  NS_TIMELINE_ENTER("setup registry");

You could leave this and move it inside the DEBUG, unless you're also
making timeline notes in autoreg itself.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 94654 [details] [diff] [review]
proposed patch v.1

r=dveditz with those minor changes

Comment 13

[Doug Turner (:dougt)]

Checking in embedding/base/nsEmbedAPI.cpp;
/cvsroot/mozilla/embedding/base/nsEmbedAPI.cpp,v  <--  nsEmbedAPI.cpp
new revision: 1.38; previous revision: 1.37
done
Checking in xpinstall/public/nsISoftwareUpdate.h;
/cvsroot/mozilla/xpinstall/public/nsISoftwareUpdate.h,v  <--  nsISoftwareUpdate.h
new revision: 1.30; previous revision: 1.29
done
Checking in xpinstall/src/nsSoftwareUpdate.cpp;
/cvsroot/mozilla/xpinstall/src/nsSoftwareUpdate.cpp,v  <--  nsSoftwareUpdate.cpp
new revision: 1.98; previous revision: 1.97
done
Checking in xpcom/build/nsXPComInit.cpp;
/cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v  <--  nsXPComInit.cpp
new revision: 1.152; previous revision: 1.151
done
Checking in xpfe/bootstrap/nsAppRunner.cpp;
/cvsroot/mozilla/xpfe/bootstrap/nsAppRunner.cpp,v  <--  nsAppRunner.cpp
new revision: 1.363; previous revision: 1.362
done
Checking in nsSoftwareUpdate.h;
/cvsroot/mozilla/xpinstall/src/nsSoftwareUpdate.h,v  <--  nsSoftwareUpdate.h
new revision: 1.40; previous revision: 1.39
done

Comment 14

[Chak Nanga]

Reopening this since it busts MRE based embedding clients in the release mode.
Now, when i run MfcEmbed (from the nightly builds which are in release mode) in
an MRE based env. the app silently fails to startup i.e. nothing happens.

I did not realize until now that we've #ifdef'd the MRE AutoRegister portion of
the code as well at
http://lxr.mozilla.org/seamonkey/source/embedding/base/nsEmbedAPI.cpp#112

Comment 15

[Adam Lock]

Would putting a .autoreg file in the mre dist fix this?

Or how about a patch to check if compreg.dat exists? Obviously if there is no
such file we *should* autoreg no matter what anyone says because xpcom would be
pretty useless without it.

Comment 16

[Brian Ryner (not reading)]

We should probably always autoregister if no component registry exists.

Comment 17

[Chak Nanga]

I think we always autoregister if the component registry does not exist or if it
finds a .autoreg file. The issue is it does not do the MRE specific
autoregistration call (which got commented out as part of this fix).

I'll submit a patch in a sec..

Comment 18

[Chak Nanga]

Attachment: Patch to autoregister MRE components if a component registry does not exist

Basically, all i'm doing is adding the MRE registration code to
NS_InitXPCom2(). 
This got commented out as part of the following earlier patch (to
NS_InitEmbedding() :
http://bugzilla.mozilla.org/attachment.cgi?id=94658&action=view

Comment 19

[Brian Ryner (not reading)]

Comment on attachment 96343 [details] [diff] [review]
Patch to autoregister MRE components if a component registry does not exist

sr=bryner

Comment 20

[Darin Fisher]

Comment on attachment 96343 [details] [diff] [review]
Patch to autoregister MRE components if a component registry does not exist

>Index: nsXPComInit.cpp

>+                    NS_ASSERTION(PR_FALSE, "Could not AutoRegister MRE components");

same as NS_ERROR("...");

r=darin

Comment 21

[Chak Nanga]

Patch checked in (using NS_ERROR per Darin's suggestion now)

Thanks bryner/darin for the reviews..

Comment 22

[Michael Dunn]

To ashish for bug verification

Comment 23

[Ashish Bhatt]

marking as verified