119491 - [ps] Cleanup global vars in PostScript module (gfx/src/ps/)

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P2)

Description

[rods (gone)]

Comment 1

[Roland Mainz]

Requires patch in bug 112635 to checked-in first...

Comment 2

[Roland Mainz]

*** Bug 119492 has been marked as a duplicate of this bug. ***

Comment 3

[Roland Mainz]

I'll file a combined patch for both Xprint and PostScript modules, fixing
summary...

Comment 4

[Roland Mainz]

Sorry, I did not had enougth time for this, retargeting to Mozilla 1.1 for now.

rods:
Is that OK for you ? If not please adjust the milestone setting... :)

Comment 5

[Samir Gehani]

Nominating for mozilla1.0 (and nsbeta1) since this means printing from print
preview will be disabled on unices for mozilla1.0 if we don't fix this bug.  

Comment 6

[Roland Mainz]

Fixing some fields (milestone, component, QA, etc.) ...

Comment 7

[scottputterman]

nsbeta1- per ADT triage. 

Comment 8

[Boris Zbarsky [:bzbarsky]]

It seems to me that the comments in this bug were not clear as to the impact...
Until this bug is fixed, there is no way to

1)  Print two documents in parallel
2)  Print from print preview
3)  Zoom in print preview
4)  Change postscript/landscape mode in print preview

This bug effectively makes print preview useless....

Comment 9

[Roland Mainz]

putterman wrote:
> nsbeta1- per ADT triage.

... ... what does that mean ?!

Comment 10

[Boris Zbarsky [:bzbarsky]]

That means "Netscape does not care whether print preview works on Unix".

Comment 11

[Chu Alan #2]

Boris: 5) plus some crash problem in print preview :) (Unix only ?)

Comment 12

[Peter Trudelle]

Rather than make assertions about what Netscape cares about (after all, we did
provide engineering time to get this working on Linux), it is better to provide
clear information in the bug as to the actual impact, as you have done.

The information in 1-5 may not have been clear to ADT at triage time.,
re-nominating to get ADT to consider new data.

Comment 13

[Roland Mainz]

Short: I am currently working on a patch for this...

Comment 14

[Samir Gehani]

Roland,
Which milestone do you plan on fixing this for?  Thanks.

Comment 15

[Roland Mainz]

Samir Gehani wrote:
> Which milestone do you plan on fixing this for?

Unfortunatly - for one half of this bug the answer is:
"Never. It's not possible without rewriting the PostScript module from
scratch..." ;-(

The problem that we cannot safely use more than one |nsDeviceContextPS| is
deeply rooted in the fact that the PostScript module code makes use of global
vars - lots of global vars - which are spread wildly all over the whole source
(~48000lines of code).
My first idea of making the code reentrant was simply to create a handle-API for
these globals, however this strategy fails at the point that the global vars are
in nearly every piece of code (sure, we could just add lots of "handle"-classes
- but that will end in the issue that the code is getting more and more complex
then, making it a pain for those who will have to handle the source in the years
to come).
It is still possible to fix the issue, however we would have to dismantle,
cleanup and redesign at least 3/4 of the code - a single engineer will need _at_
_least_ a month (assuming he/she is working 24h/day, e.g. a single person would
take 2-3months in real life) for this work (incl. testing if the patch really
works - which needs someone with Rational Purify and a new breed of test
application in gfx/tests/ which runs multiple device instances in parallel with
input and then compares the output, too).

After playing around with the code and seeking for possible solutions for the
last couple of months the only usefull solution is (unless someone steps up and
wants to spend the next three months with trying the hacked solution described
above) IMHO to abadon the current code in gfx/src/ps/ and rebuild a new version
in gfx/src/ps2/ (or gfx/src/postscript/) while having the terms "global r/w-vars
are forbidden", "code must be reentrant" and "threadsafety" in mind (during the
design phase, please :)

Comment 16

[Roland Mainz]

I've fixed the Xprint module code to support multiple device instances as part
of bug 187125 so doing multiple print jobs concurrently to different printers or
doing printing and print preview at the same time works with that module.

I'll leave the bug open for the code in gfx/src/ps/, however I do not have the
time to rewrite that code from scratch to support that feature.

Comment 17

[kill this account]

Louie/Pete: any interest?

Comment 18

[Kevin McCluskey (gone)]

nsbeta1-. Not enough resources to go after this for next release.

-> kin, since Rod is no longer working on printing issues.

Comment 19

[Pete Zha]

>Louie/Pete: any interest?
We will find some time to do this. As comment 15, there are two ways. Rewrite or
reentrant the code. Both way needs lots of time. I'm not sure which one is better. 

Comment 20

[kill this account]

What about using a external process like Ghostview?

Ghostview gives an accurate view of what is going to be printed.

Comment 21

[Boris Zbarsky [:bzbarsky]]

That does not allow you to dynamically see the effects of changing
between landscape and postscript does it?  Since the PS would have
to just be generated then passed off to the helper....

It also does not resolve the problems we currently have if you try to
print while another (long) document is printing.

Comment 22

[kill this account]

> That does not allow you to dynamically see the effects of changing
> between landscape and postscript does it?  Since the PS would have
> to just be generated then passed off to the helper....

This is not clear. It looks like Ghostview uses (wraps) ghostscript. I
know for certian that when Ghostview is displaying from a file it is
definitely able to update the image when the file changes. Gecko would, of 
course, still be responsible for producing the postscript but the helper 
process could be automatically informed of the new output and re-render it.

> It also does not resolve the problems we currently have if you try to
> print while another (long) document is printing.

Yes, of course, the globals will need to be cleaned up to allow multiple 
prints to occur simultaneously.

However, using Ghostview (or some part of it) to wrap ghostview could give 
us much simplier code along with an accurate rendering of the output.

Comment 23

[Kenneth Herron]

Taking.

Comment 24

[Kenneth Herron]

Attachment: Replace global nsIPref object

This replaces the global nsIPref object in nsPostScriptObj.cpp with
nsIPrefService & nsIPrefBranch objects created as needed.

Could someone volunteer to review this? No postscript knowledge is required. I
hate to keep leaning on the same two reviewers all the time.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136963 [details] [diff] [review]
Replace global nsIPref object

I know caillon wants to review this.. ;)

Comment 26

[Kenneth Herron]

Attachment: Replace global font debugging flag

This removes a font debugging flag from nsDeviceContextPS.cpp and
nsFontMetricsPS.cpp. The debugging functionality is performed through nspr's
logging subsystem instead.

Once this is checked in, the file gfx/src/ps/nsFontPSDebug.h will no longer be
needed, and should be removed from the source tree.

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 137024 [details] [diff] [review]
Replace global font debugging flag

nsFontPSDebug.h can be removed now, right?

+static PRLogModuleInfo *FontMetricsPSM = PR_NewLogModule("FontMetricsPS");
please name the variable gFontMetricsPSM

Comment 28

[Kenneth Herron]

Attachment: Font debugging flag, take two

Renamed FontMetricsPSM -> gFontMetricsPSM as requested.

Yes, as noted gfx/src/ps/nsFontPSDebug.h should be removed when this patch is
checked in.

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 136963 [details] [diff] [review]
Replace global nsIPref object

>+static void
>+EnumerateLangPrefs(char **plist, PRUint32 pcount, unsigned int prefixlen,
>+  nsIPrefBranch *prefbranch, nsPostScriptObj *psobj)
>+{
>+  NS_PRECONDITION(plist != nsnull, "pref list is null");
>+  for (int i = pcount; i--; ) {
>+    NS_ASSERTION(plist[i] != nsnull, "pref list contains null");
>+    PrefEnumCallback(plist[i] + prefixlen, prefbranch, psobj);
>+    nsMemory::Free(plist[i]);
>+  }
>+  nsMemory::Free(plist);

While this "works", I think it is pretty shady for this method to free memory
which it doesn't control, especially without advertising it.  I'd rather you
just call NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY from initlanggroup() which is
where this memory is being managed.  Actually, I think that this method can go
altogether.  It isn't called enough IMO to warrant its existence.  Just move
the functionality here into its caller.

Comment 30

[Boris Zbarsky [:bzbarsky]]

I'm not likely to get to this before Jan 15...  Sorry about that.

Comment 31

[Kenneth Herron]

I started to make the changes described in comment 29, but I'm afraid I just
don't agree with them.

|initlanggroup()| has to iterate over two separate arrays of preference strings,
performing identical operations on each. Factoring the iteration code into a
separate function is a good development practice. It reduces LOC, it reduces
compiled object size, and it makes |initlanggroup()| more comprehensible by
separating the high-level process of initializing lang groups from the low-level
process of iterating through an array. I don't see a benefit to rolling the code
from |EnumerateLangPrefs()| directly into |initlanggroup()|.

Regarding use of |NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY()|, normally I'm in
favor of using high-level functions as much as possible, but I don't see the
point here. NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY() requires the caller to
specify the array size and (perhaps implicitly) the function for freeing array
objects, so it doesn't provide information hiding. It's a macro, so it doesn't
reduce code size. It adds a loop with an iteration variable everywhere it's
called. And for some reason it takes a signed int for the array size, which
means converting the unsigned int returned from the code that allocated the
array in the first place. The existing code already iterates over the array
being freed, and contains places where it's natural to free the allocated items.
 I don't see any value to changing the code as written.

Regarding the fact that memory allocated in |initlanggroup()| is freed in
|EnumLangPrefs()|, this is of course because |EnumLangPrefs()| contains the code
which iterates over the array. |EnumLangPrefs()| is documented as being a helper
function for initlanggroup(), it's not called by anywhere else, and the
operation that it performs is very specific to |initlanggroup()|'s needs. In
this case, I don't see a problem with the distribution of work. I can see
amending the description for |EnumerateLangPrefs()| to to mention that the pref
string array is freed.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 137766 [details] [diff] [review]
Font debugging flag, take two

>Index: gfx/src/ps/nsFontMetricsPS.cpp
>-          if (gFontPSDebug & NS_FONTPS_DEBUG_FIND_FONT) {
>+          if (PR_LOG_TEST(gFontMetricsPSM, PR_LOG_DEBUG)) {

This whole block should probably go inside #ifdef PR_LOGGING

>+    if (PR_LOG_TEST(gFontMetricsPSM, PR_LOG_DEBUG)) {
>+      nsCAutoString fontname, stylename;
>       entry->GetFamilyName(fontname);
>       entry->GetStyleName(stylename);
>+      PR_LogPrint("    -- %s '%s/%s'\n", (loaded ? "already loaded" : "load"),
>+          fontname.get(), stylename.get());
>     }

Same.

sr=bzbarsky with those two changes.  Sorry this took so long....

Comment 33

[Kenneth Herron]

Attachment: Font debugging flag, take three

The indicated sections have been wrapped by #ifdef PR_LOGGING. A couple of
variables were only used inside the ifdef'ed code so I also moved their
declaration.

Again, gfx/src/ps/nsFontPSDebug.h should be removed when this is checked in.

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

patch checked in, including removal of nsFontPSDebug.h. leaving open per
kherron's request.

Comment 35

[Kenneth Herron]

Resolving fixed. This was kept open for additional work work on the remaining global variables in the module. The code at issue is obsolete on the trunk, and only major bug fixes are being taken on the branches.