204145 - too many nsXULPDGlobalObjects with attendant JSContexts

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[Brendan Eich [:brendan]]

Bryner knows about this, we've seen more than 50 JSContexts in the one and only
runtime, and most of them came from chrome XUL documents.  Each such document
has an nsXULPrototypeDocument, which has an nsXULPDGlobalObject, which has an
nsIScriptContext implementation (nsJSContext), which has a JSContext.  All so
the DOM nsJSEnvironment.cpp code (directly or indirectly via caps code) can ask
a global object for its principal.

In the case of chrome-loaded XUL, that principal is always the system principal.
 We don't need all these steenkin' objects just to return a singleton!

Patch in a moment.

/be

Comment 1

[Brendan Eich [:brendan]]

Attachment: proposed fix

Magic eight-ball says ben should r= and bryner sr=, with dbaron kibbitzing.

/be

Comment 2

[Brian Ryner (not reading)]

Comment on attachment 122275 [details] [diff] [review]
proposed fix

>Index: nsXULPrototypeDocument.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xul/document/src/nsXULPrototypeDocument.cpp,v
>retrieving revision 1.50
>diff -p -u -8 -r1.50 nsXULPrototypeDocument.cpp
>--- nsXULPrototypeDocument.cpp	19 Apr 2003 00:28:05 -0000	1.50
>+++ nsXULPrototypeDocument.cpp	1 May 2003 23:15:09 -0000
>-    rv |= NS_ReadOptionalObject(aStream, PR_TRUE, getter_AddRefs(mDocumentPrincipal));
>-    if (!mDocumentPrincipal) {
>-        // XXX This should be handled by the security manager, see bug 160042
>-        PRBool isChrome = PR_FALSE;
>-        if (NS_SUCCEEDED(mURI->SchemeIs("chrome", &isChrome)) && isChrome)
>-            rv |= securityManager->GetSystemPrincipal(getter_AddRefs(mDocumentPrincipal));
>-        else
>-            rv |= securityManager->GetCodebasePrincipal(mURI, getter_AddRefs(mDocumentPrincipal));
>+    nsCOMPtr<nsIPrincipal> principal;
>+    rv |= NS_ReadOptionalObject(aStream, PR_TRUE, getter_AddRefs(principal));
>+    if (! principal) {
>+        rv |= GetDocumentPrincipal(getter_AddRefs(principal));

Nit: try to be consistent about whether you use braces for single-line |if|
blocks.


>@@ -895,16 +939,21 @@ nsXULPDGlobalObject::SetScriptsEnabled(P
> //
> // nsIScriptObjectPrincipal methods
> //
> 
> NS_IMETHODIMP
> nsXULPDGlobalObject::GetPrincipal(nsIPrincipal** aPrincipal)
> {
>     if (!mGlobalObjectOwner) {
>+        if (this == nsXULPrototypeDocument::gSystemGlobal) {
>+            *aPrincipal = nsXULPrototypeDocument::gSystemPrincipal;

Is it possible for gSystemPrincipal to not yet be initialized?

sr=bryner with those addressed.

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 122275 [details] [diff] [review]
proposed fix

r=[e-mail]

Comment 4

[Brendan Eich [:brendan]]

bryner: I consistently do not brace single-line then and else clauses, unless
the other clause (for an if-else) is multi-line, *or* the if condition is
multi-line (comments count in making any part multi-line).

The neat thing about the bootstrapping is that, because
nsXULPrototypeDocument::NewXULPDGlobalObject calls
nsXULPrototypeDocument::GetDocumentPrincipal before possibly setting
gSystemGlobal, and because it sets gSystemGlobal only if (a)
GetDocumentPrincipal succeeds and (b) its result == gSystemPrincipal (which
GetDocumentPrincipal sets), we know that gSystemGlobal => gSystemPrincipal.

Reviewers, anyone: please run my patch!  Works for me, but more testing would be
great.

/be

Comment 5

[Brendan Eich [:brendan]]

I wonder how much of a Ts gain this is.  jrgm, is there a good machine to test
the patch on and measure Ts?

/be

Comment 6

[John Morrison]

Yes, I have a 400MHz/128MB machine, dual-boot to win98 or rh7.1. I have a build 
tree on either, with linux relatively current, but win98 is somewhat old 
(although I can just update my opt tree on win2k and copy over to the other 
machine).

Comment 7

[John Morrison]

I had a look at this on win98/400MHz/128MB and win2k/500MHz/128MB. I don't see 
a gain or loss on win2k for either warm or cold start. On win98, I have the 
paradox that warm start is maybe 1% slower, but the cold start is (reportedly)
up to second slower. Uh, ignore that result. I expect I screwed the test 
somehow; I'll try to test this out again later (and on linux).

Comment 8

[John Morrison]

Oh, right. I nuked a 'Cache.Trash' folder when cleaning up the profile to run 
the second "faster" cold start test. That's probably (?) the reason for the bad 
result.

Comment 9

[Brendan Eich [:brendan]]

Fixed.

/be