[ps] Huge static buffer in gfx/src/ps/nsPostScriptObj.cpp

VERIFIED FIXED

Status

()

--
minor
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: kherron+mozilla, Assigned: kherron+mozilla)

Tracking

({memory-footprint})

Trunk
Sun
Solaris
memory-footprint
Points:
---
Bug Flags:
blocking1.4a -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

16 years ago
The fix for bug 161634 introduced a static buffer into nsPostScriptObj.cpp,
named "envvar". This buffer is used to hold an environment variable string, i.e.
"MOZ_PRINTER_NAME=<name of printer>".

The buffer is of size ARG_MAX from <limits.h>. ARG_MAX is the maximum size of
argument & environment data passed to a new process via exec(). On solaris
ARG_MAX is 1,048,320 bytes (double that in the 64-bit environment); on
(Mandrake) linux it's 131,072 bytes. This is wasteful for a buffer expected to
contain a single environment variable.
(Assignee)

Comment 1

16 years ago
Created attachment 115402 [details] [diff] [review]
Smaller buffer for MOZ_PRINTER_NAME

This patch shrinks the environment variable buffer to be big enough for a
64-character printer name. Allocating this buffer dynamically seems more
trouble than it's worth. The code already avoided overrunning the buffer or
truncating the printer name, so the only shortcoming is that really long
printer names aren't supported.  You could double or quadruple the buffer here
and still save space over the existing code.

The patch also reduces the number of literal strings required.

> size nsPostScriptObj.o.*
nsPostScriptObj.o.new: 200497 + 2920 + 88 = 203505
nsPostScriptObj.o.old: 200597 + 2920 + 1048324 = 1251841

Updated

16 years ago
Summary: Huge static buffer in gfx/src/ps/nsPostScriptObj.cpp → [ps] Huge static buffer in gfx/src/ps/nsPostScriptObj.cpp
Doh.  :(  my bad.....
Assignee: rods → Roland.Mainz
Flags: blocking1.4a?

Comment 3

16 years ago
good catch.. the fix seems ok but it seems like somehow using an nsCAutoString
somewhere would be better...(obviously this is static so we'd need some logic
around it)
(Assignee)

Comment 4

16 years ago
Created attachment 115538 [details] [diff] [review]
2nd try - embrace mozilla's string classes

This uses a static nsCAutoString to store the environment variable. It compiles
& links fine, but I'm inexperienced with mozilla's string classes so there
could be some newbie mistakes.

> size nsPostScriptObj.o
200937 + 2920 + 88 = 203945

Comment 5

16 years ago
Comment on attachment 115538 [details] [diff] [review]
2nd try - embrace mozilla's string classes

this looks fine. just for everyone reading, static classes are ok when they
appear inside functions because they don't have to be initialized by the
linker.
sr=alecf
Attachment #115538 - Flags: superreview+
(Assignee)

Updated

16 years ago
Attachment #115538 - Flags: review?(bzbarsky)
(Assignee)

Updated

16 years ago
Attachment #115538 - Flags: review?(bzbarsky) → review?(dcone)
Comment on attachment 115538 [details] [diff] [review]
2nd try - embrace mozilla's string classes

Dunno if dcone will ever even look at this.. r=me if you need it.
static classes really aren't very good if we ever want to unload libraries,
since some old-fashioned compilers (gcc 2.x on x86/Linux and probably some other
platforms, when not using -fuse-cxa-atexit) call the destructors using |atexit|.
(Also note that a |static nsCAutoString| would likely get a different buffer for
different calls to this function.  That's probably bad as well.)
Comment on attachment 115402 [details] [diff] [review]
Smaller buffer for MOZ_PRINTER_NAME

sr=dbaron on this patch (the first), although it might be good to:
 * move |printer_env|  and |printer|pat| down closer to where they're used.

I'm also not sure whether the %s is as good as
dummy_value_to_make_putenv_happy, or why that was needed at all.
Attachment #115402 - Flags: superreview+

Updated

16 years ago
Flags: blocking1.4a? → blocking1.4a-
Kenneth Herron: Can you attach a new patch that fixes comment#9 ?
You got r= from bz with comment #6
(Assignee)

Comment 11

16 years ago
Created attachment 122355 [details] [diff] [review]
3rd try - Dynamically allocate space for the envvar

This patch dynamically allocates a buffer for the new environment string, puts
the new buffer in the environment, and then frees any previously allocated
buffer. Any size printer name is supported without wasting any memory.

This design is based on comment #7. Unloading the library would invalidate any
static buffers defined within the library; this would be bad news if one of
those buffers was hooked into the environment. But a dynamically allocated
string would remain valid.

I gather that unloading libraries isn't supported right now so I haven't
addressed the potential memory leak in that event. I'm not sure what could be
done anyway. NSPR doesn't document any method to remove an envvar from the
environment without replacing it with another envvar. So after the envvar has
been created, some kind of buffer would have to be maintained for the life of
the process.
(Assignee)

Updated

16 years ago
Attachment #122355 - Flags: superreview?(dbaron)
Attachment #122355 - Flags: review?(bz-bugspam)
Comment on attachment 122355 [details] [diff] [review]
3rd try - Dynamically allocate space for the envvar

Fix the indent on that error return statement, and r=bzbarsky... but please
file a bug on NSPR about the inability to properly clean up env vars, and
reference that bug in the code comments here and this bug in the opening
comment of the NSPR bug; once that's fixed we should update this code.
Attachment #122355 - Flags: review?(bz-bugspam) → review+
Comment on attachment 122355 [details] [diff] [review]
3rd try - Dynamically allocate space for the envvar

> #include <limits.h>

Does limits.h still need to be included?
(Assignee)

Comment 14

16 years ago
Created attachment 122505 [details] [diff] [review]
Dynamically allocate space - removed <limits.h>

Christopher is correct; <limits.h> is no longer needed. This also fixes the
stray tab character.

Bug 204494 is an RFE to add NSPR support for removing environment variables.
Attachment #122355 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #122505 - Flags: superreview?(dbaron)
Attachment #122505 - Flags: review?(bz-bugspam)
Attachment #122505 - Flags: review?(bz-bugspam) → review+

Comment 15

16 years ago
Reassigning to Kenneth Herron...
Assignee: Roland.Mainz → kjh-4275
Comment on attachment 122505 [details] [diff] [review]
Dynamically allocate space - removed <limits.h>

Initially I didn't like this approach, but I guess it's ok.  sr=dbaron.
Attachment #122505 - Flags: superreview?(dbaron) → superreview+
Checking in gfx/src/ps/nsPostScriptObj.cpp;
/cvsroot/mozilla/gfx/src/ps/nsPostScriptObj.cpp,v  <--  nsPostScriptObj.cpp
new revision: 1.90; previous revision: 1.89
done
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

15 years ago
Verified fixed. Thanks for checking this in, Christian.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.