201891 - Lazily instantiate CSSFrameConstructor::gXBLService

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Scott MacGregor]

This is fall out from Bug #194568 where creating two separate instances of the
XBL service leads to an infinite loop. We'd like to break the cycle described in
that bug. 

This means lazily instantiating CSSFrameConstructor::gXBLService, so we don't
create it when the layout module is loaded. 

I'll attach a non optimal patch which I used to make sure this fixes an infinite
loop I run into with Minotaur.

Comment 1

[Scott MacGregor]

Attachment: a possible patch

Here's a test patch I used to make sure this fixed the problem I was seeing. I
removed the call to InitGlobals when the layout module is loaded. I added a new
static method called GetXBLService which initializes and returns gXBLSerice.
(it does not reference the object on the return). Replaced the callers of
gXBLService with GetXBLService. 

ReleaseGlobals is still called from the layout module when it is being
unloaded.  that's the one part that still seems weird with this test patch of
mine. We have a method called GetXBLService which is paired with a
ReleaseGlobals method. A naming inconsistency.

Comment 2

[Scott MacGregor]

David, can you comment about how you'd like to see this fixed? Read my comments
above about why I think my patch may not be appropriate to be checked in. I'd
like your input on how you'd like to resolve the inconsistency of when we
instantiate and when we release the XBL service.

Comment 3

[Mike Kaply [:mkaply]]

Scott,

I see a crash with a NULL gXULCache in this case, why are you seeing an infinite
loop?

The OS/2 Tbird build crashes at startup on the first invocation.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 120397 [details] [diff] [review]
a possible patch


The inconistency doesn't bother me.  We do that kind of thing all over the
place.

A few comments on the patch, though:

>Index: html/style/src/nsCSSFrameConstructor.cpp
>@@ -1370,6 +1370,18 @@
>   return NS_OK;
> }
> 
>+nsIXBLService * nsCSSFrameConstructor::GetXBLService()
>+{
>+  nsresult rv = NS_OK;
>+  if (!gXBLService) 
>+    rv = CallGetService("@mozilla.org/xbl;1", &gXBLService);
>+  
>+  if (NS_SUCCEEDED(rv))
>+    return gXBLService;
>+  else
>+    return NULL;

Why not make this a little simpler by doing:

if (!gXBLService) {
  nsresult rv = CallGetService("@mozilla.org/xbl;1", &gXBLService);
  if (NS_FAILED(rv))
    gXBLService = nsnull;
}
return gXBLService;

>Index: html/style/src/nsCSSFrameConstructor.h
>+  static nsresult InitGlobals() { if (!gXBLService) return CallGetService("@mozilla.org/xbl;1", &gXBLService); else return NS_OK; }

Nothing's calling this anymore, so get rid of it.

Comment 5

[Scott MacGregor]

Thanks David, I'll update the patch tonight with the suggestions you made then
drive it into the tree.

Comment 6

[Scott MacGregor]

Attachment: updated patch with dbaron's comments

Comment 7

[Scott MacGregor]

This has been checked in. 

Mike did I just read in your bug comment that you have thunderbird running on
OS2? That's pretty cool. I'm not sure why OS2 was crashing. On windows and
linux, we spun in an infinite loop. See the discussion in Bug #194568 for
information about why.

Comment 8

[jag (Peter Annema)]

if NS_FAILED(rv) for CallGetService, isn't gXBLService always going to be nsnull?

If not, should we fix CallGetService (and/or |::GetService(ByContractID)|) to
make that assumption true?