If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Fastload landing increased size of principals structs

RESOLVED FIXED in mozilla0.9.4

Status

SeaMonkey
General
P1
normal
RESOLVED FIXED
16 years ago
13 years ago

People

(Reporter: Daniel Bratell, Assigned: brendan)

Tracking

Trunk
mozilla0.9.4
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

16 years ago
The leak figures on Tinderbox increased after the fastload landing:


--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsXULPDGlobalObject                              56          -
nsSystemPrincipal                                56       7.69%
nsCodebasePrincipal                              64       6.67%
TOTAL                                           176
--NEW-BLOAT-----------------------------------bloat------bloat%-----------------------
nsFastLoadService                                36          -
nsThreadPoolRunnable                             32     100.00%
nsXULPrototypeCache                             188      30.56%
nsThread                                        140      25.00%
nsXULPrototypeDocument                         1344      20.00%
nsJARURI                                       5376      14.38%
nsSystemPrincipal                                56       7.69%
nsCodebasePrincipal                            1216       6.67%
nsAggregatePrincipal                            684       5.56%
nsJSCID                                        3536       4.62%
nsROCSSPrimitiveValue                          5096       2.08%
nsComputedDOMStyle                             3200       2.04%
XPCWrappedNativeProto                          3192       1.79%
xptiInterfaceInfo                            103572       1.05%
nsFactoryEntry                                57816       0.63%
nsGenericFactory                               4420       0.45%
nsHashtable                                   69608       0.06%
nsCParserNode                                861600       0.01%
TOTAL                                       1121112
(Assignee)

Comment 1

16 years ago
I bet the principals are from the XUL script cache.  Since FastLoad is pref'd
off by default, I am not sure why nsFastLoadService is leaking.  Daniel, did you
enable the pref (set nglayout.debug.disable_xul_fastload to false)?

/be
No, it's just reporting that the size of the principals is 4 bytes bigger.

The only new leak here is the nsXULPDGlobalObject.  We shouldn't be leaking
that, though...
(Assignee)

Comment 3

16 years ago
I interposed nsISerializable between nsIPrincipal and nsISupports on each
principal type's inheritance chain -- didn't that save another vtbl ptr?  I did
not add data members to any principals concrete class.

/be
It's not getting GC'ed.  The leaked GC roots at shutdown look like they're just
the usual |nsXULPrototypeScript::mJSObject|s, although maybe they aren't usual
and I've been looking at too many leaks.
So I can find it, fastload is bug 68045.
(Assignee)

Comment 6

16 years ago
So if the XUL prototype cache survives past XPCOM shutdown, we'll leak the
XULPDGlobalObject, and I bet that's now happening.  But why?

/be
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.4
(Assignee)

Comment 7

16 years ago
Probably a dup of bug 93146, although why did the principals structs bloat?

/be
Yes, the leaks were a dup of bug 93146, so retitling this bug.
Summary: Fastload landing increases tinderbox leaks → Fastload landing increased size of principals structs
(Assignee)

Comment 9

16 years ago
I added an encode function pointer to JSPrincipals, and forgot. That is what
made the nsBasePrincipal subclasses grow by four bytes. Closing as INVALID, not
a bug (not enough bloat to fear).

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → INVALID
(Assignee)

Comment 10

16 years ago
I was mindlessly following the pattern established long ago of adding function
pointers to JSPrincipals, but there is no opportunity to vary principals->encode
among several functions, or have several principals each have its own function
pointer different from the others.  The principalsDecoder member of JSRuntime is
another strong clue that only one serialization system at a time can use the
engine's API.

Patch coming up to recover the wasted space.

BTW, the other function pointers in JSPrincipals never vary, and could be moved
to a single "ops" struct (poor man's C vtbl) or to the JSRuntime.  But given the
age of those members, I'm concerned someone out there is using them, and may
even be married to the JSPrincipals API.

/be
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Updated

16 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 11

16 years ago
Created attachment 44959 [details] [diff] [review]
proposed fix: remove encode from JSPrincipals, make rt->principalsDecoder a transcoder
(Assignee)

Comment 12

16 years ago
Created attachment 44960 [details] [diff] [review]
diff -wu version of patch
(Assignee)

Comment 13

16 years ago
Review craved.  The diff -wu is easier to read, as in fusing
nsEncodeJSPrincipals and nsDecodeJSPrincipals in caps/src/nsJSPrincipals.cpp, I
had to indent the code more, and avoid early returns.

/be
r/sr=jst
r=shaver, with the s/Xcode/Transcode/g change from IRC.

Comment 16

16 years ago
I guess I mentioned this on IRC last night but it must've been missed. I don't
see where the space reduction is coming from in this patch?

Comment 17

16 years ago
Never mind: shaver showed me.
(Assignee)

Comment 18

16 years ago
Fixed.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.