44790 - TestGtkEmbed crashes on startup

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P3)

Description

[Inaky Perez Gonzalez]

Problem when running TestGtkEmbdedded in latest CVS mozilla
versions is the chrome service is trying to load a file
(dist/bin/installed-chromes.txt) that doesn't exist. This causes the
GtkMozEmbed structure not to be created/initialized by
gtk_moz_embed_init(), what crashes the whole GtkEmbed system.

        Some times it would work, but I haven't been able to isolate
them or to come out with a reproducible test case.

        The problem is shown when executing function
nsChromeRegistry::CheckForNewChrome(), which in turn is called from
function NS_InitEmbedding(), at file
embedding/base/nsEmbedAPI.cpp. The return value for that function is
the return value of nsChromeRegistry::CheckForNewChrome(). As the
named file doesn't exist it will always fail.

        So I went to the normal mozilla startup sequence, and found that
this return value is completely ignored (function main1() at
xpfe/bootstrap/nsAppRunner.cpp). There is a call to function
CheckForNewChrome(), which in turn creates a new chrome registry and
if successful, calls nsChromeRegistry::CheckForNewChrome(), returning
it's result.

        But in this case, that value is ignored.

        So, my sollution to make it work, fast and easy, has been to
ignore that return code in NS_InitEmbedding(). It seems to work,
though there are still some problems, but I haven't been able to
associate them with this proposed sollution.

	This patch provides it:

diff -u -r1.7 embedding/base/nsEmbedAPI.cpp
--- embedding/base/nsEmbedAPI.cpp 2000/07/04 21:53:24 1.7
+++ embedding/base/nsEmbedAPI.cpp 2000/07/07 18:01:38
@@ -170,8 +170,8 @@
     if (!chromeReg)
         return NS_ERROR_FAILURE;
 
-    return chromeReg->CheckForNewChrome();
-
+    chromeReg->CheckForNewChrome();
+    return;
 }

Comment 1

[Judson Valeski]

cc'ing

Comment 2

[Christopher Blizzard (:blizzard)]

Actually, here's the patch I have in my tree.  ( the function should return
something. )  I sent email to hyatt + adam about this problem since I don't
think that the call should return an error.

Index: nsEmbedAPI.cpp
===================================================================
RCS file: /cvsroot/mozilla/embedding/base/nsEmbedAPI.cpp,v
retrieving revision 1.7
diff -u -r1.7 nsEmbedAPI.cpp
--- nsEmbedAPI.cpp      2000/07/04 21:53:24     1.7
+++ nsEmbedAPI.cpp      2000/07/07 18:52:03
@@ -163,14 +163,17 @@
 
 
     // Init the chrome registry.
-    
     nsCOMPtr <nsIChromeRegistry> chromeReg =
do_GetService("component://netscape/chrome/chrome-registry");
     NS_ASSERTION(chromeReg, "chrome check couldn't get the chrome registry");
 
     if (!chromeReg)
         return NS_ERROR_FAILURE;
 
-    return chromeReg->CheckForNewChrome();
+    // ignore the return value here.  if chrome is already
+    // initialized, this will return an error.
+    chromeReg->CheckForNewChrome();
+
+    return rv;
 
 }
 

Comment 3

[Doug Turner (:dougt)]

blizzard, r=dougt.  sorry about that.

Comment 4

[Brendan Eich [:brendan]]

Well, what set rv to a good value?  I see rv tested just after being set in two 
predecessor basic blocks:

http://lxr.mozilla.org/seamonkey/source/embedding/base/nsEmbedAPI.cpp#127 (which 
should use NS_FAILED(rv) rather than NS_OK != rv, eh?), and
http://lxr.mozilla.org/seamonkey/source/embedding/base/nsEmbedAPI.cpp#145

The last is troubling, as a GetService failure does not result in an early 
return.  Do you really want that rv to propagate?  If so, do you really want to 
CheckForNewChrome (and what about the HACK_AROUND_THREADING_ISSUES section that 
occurs whether or not rv is a success code)?

While I'm nagging, what's wrong with NS_WITH_SERVICE instead of the manual, 
verbose Get/ReleaseService way?

/be 

Comment 5

[Christopher Blizzard (:blizzard)]

The GetService() code has been cut-n-pasted from well before there was
NS_WITH_SERVICE.  I'll fix that up and just clean up the function in general and
resubmit.  Might as well clean it up while I'm fixing that other problem.

Comment 6

[Christopher Blizzard (:blizzard)]

Mine, all mine.

Comment 7

[Christopher Blizzard (:blizzard)]

Attachment: patch

Comment 8

[Christopher Blizzard (:blizzard)]

Ok, I fixed a few 80th column violations, used NS_WITH_SERVICE for the event
queue and cleaned up some other styleistic things.  I left the string bundle
service failure optional since we might not want that for embedding.  Should
that be the case?  Anyone want to comment?

Comment 9

[Brendan Eich [:brendan]]

Better -- but are those hard tabs in the patch?  That doesn't square with the 
modeline comment.  Also, the modeline claims c-basic-offset is 2, but it's 
clearly 4.  Bliz, can you expand all tabs to the right stop modulus and fix 
c-basic-offset?  Thanks,

/be

Comment 10

[Christopher Blizzard (:blizzard)]

Attachment: patch 2

Comment 11

[Christopher Blizzard (:blizzard)]

Fixed.  M-x untabify to the rescue.

Comment 12

[Christopher Blizzard (:blizzard)]

Got an a=brendan.  Checked in.

Comment 13

[John Morrison]

verified

Comment 14

[Daniel Veditz [:dveditz]]

Adding keyword to bugs which already show a nsbeta2 triage value in the status 
whiteboard so the queries don't get screwed up.