369816 - jssh leaks and fails to load remote script

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Chris Shoemaker]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.9) Gecko/20061221 Fedora/1.5.0.9-1.fc5 Firefox/1.5.0.9
Build Identifier: minefield

The jssh LoadURL code makes two incorrect assumptions.

1) That the stream's contentLength will be the number of bytes to read.

Problem: This is not the case when the contentType is a gzipped script.

2) That the first Read() of the stream will produce exactly the number of requested bytes.
Problem: The Read() may read fewer than the requested bytes.

Fix: Read as many bytes as the stream will give us, and ignore contentLength - using as many calls to Read() as it takes.


Reproducible: Always

Comment 1

[Chris Shoemaker]

Attachment: Read the entire stream before evaluating it and don't leak.

Comment 2

[Brian Crowder]

There's definitely at least a real memory leak in the original version of this code, so I'm confirming this bug.

Comment 3

[Brian Crowder]

Comment on attachment 254498 [details] [diff] [review]
Read the entire stream before evaluating it and don't leak.

This needs review, and I'm not sure who should rightly be harassed with that request, so I'm taking a guess at bz.  I think in reality this thing is unowned.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 254498 [details] [diff] [review]
Read the entire stream before evaluating it and don't leak.

>Index: extensions/jssh/nsJSSh.cpp
>+  nsCString buffer("");

Don't need the |("")| part.

sr=bzbarsky.  I think Alex is close to an owner, per blame.  ;)

Comment 5

[Chris Shoemaker]

This bug seems to be growing stale, although the patch still applies cleanly.

I don't know if the "sr=bzbarsky" in comment #4 means the patch has already been reviewed, or if I need to re-submit it with the |("")| removed.

In any case, I've been running with this patch (and others, see below) applied since I submitted it back in February, and it does work for me, and it does plug a rather annoying leak.

In an attempt to add some value and hopefully get these patches committed, I am resubmitting the original patch with the |("")| removed, and also submitting two more patches - one that fixes another bug, and one that simply does better logging.

I'm keeping them here, rather that opening a new bug in the hopes that whoever eventually sees them will be able to quickly apply them all, since they all touch the same code.

Comment 6

[Chris Shoemaker]

Attachment: Improve logging facility in jssh

Improve the logging facility in jssh.

I just tried to follow the pattern I found in other code.  Using this logging mechanism seemed much better than the printfs.

Comment 7

[Chris Shoemaker]

Attachment: Allow remote scripts to load in jssh, and don't leak the strings.

This is the patch that prompted the original bug file.

It still plugs the leak, and still allows remote scripts to be loaded (see comment #1).  It also depends on the LOG macro from the previous patch.

Comment 8

[Chris Shoemaker]

Attachment: Preserve utf8 in jssh eval output.

Preserve utf8 in jssh eval output.

The original code silently drops the upper byte of every JS string by using JS_GetStringBytes().  This is very bad, and often results in garbage.  :(

With this patch, it now uses JS_GetStringChars() along with NS_ConvertUTF16toUTF8() to output a valid character stream.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I don't know if the "sr=bzbarsky" in comment #4 means the patch has already
> been reviewed

Patches need two reviews.  Note that I set a review request on the patch (which you removed by marking it obsolete).

See also http://www.mozilla.org/hacking/life-cycle.html

Comment 10

[Chris Shoemaker]

(In reply to comment #9)
> Patches need two reviews.  Note that I set a review request on the patch (which
> you removed by marking it obsolete).
> 
> See also http://www.mozilla.org/hacking/life-cycle.html

I see, thanks.  So, in this case, the sr=bzbarsky meant that I shouldn't have resubmitted the patch just for the |("")|.  I kind of suspected that to be the case.  And, the instructions say that I should have gone in search for an alternate reviewer after one week.  Again, my fault.

I'll ask for review on the new patch(es) and wait another week.  Thanks.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'd try mailing Alex directly about this review, for what it's worth.

Comment 12

[Chris Shoemaker]

(In reply to comment #11)
> I'd try mailing Alex directly about this review, for what it's worth.
> 

Done.

Comment 13

[Alex Fritze]

Comment on attachment 272813 [details] [diff] [review]
Preserve utf8 in jssh eval output.

Looks good to me.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272809 [details] [diff] [review]
Improve logging facility in jssh

I don't think it's obvious from the NSPR docs that creating two modules with the same name is OK.  It happens to work, but you shouldn't depend on it.  Please use a single log module referenced from both places.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272810 [details] [diff] [review]
Allow remote scripts to load in jssh, and don't leak the strings.

>Index: extensions/jssh/nsJSSh.cpp
>+    LOG(("appended %d bytes:\n%s", bytesRead, buffer.get()));

That's not a good idea if there are null bytes in the data...  Depends on what you want here, of course.

This whole setup is pretty clunky; using ReadSegments() with the appropriate writer function that copies to a string would be better.  But this works, I guess.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272813 [details] [diff] [review]
Preserve utf8 in jssh eval output.

>Index: extensions/jssh/nsJSSh.cpp
>+         nsAutoString chars;
>+         chars.Assign(NS_REINTERPRET_CAST(const PRUnichar*, JS_GetStringChars(str)));

nsDependentString chars(reinterpret_cast<const PRUnichar*>(JS_GetStringChars(str)),
                        JS_GetStringLength(str));

>+         nsCAutoString cstr = NS_ConvertUTF16toUTF8(chars);

NS_ConvertUTF16toUTF8 cstr(chars);

with those changes, sr=bzbarsky.  Please post an updated patch?

Comment 17

[Chris Shoemaker]

Attachment: improve logging facility in jssh

Updated previous patch to factor out the logging module into nsJSSh.h.

Comment 18

[Chris Shoemaker]

Attachment: Allow remote scripts to load in jssh, and don't leak the strings.

Updated just to apply cleanly on top of previous patch.

Comment 19

[Chris Shoemaker]

Attachment: Preserve utf8 in jssh eval output.

Updated with requested changes to string construction.

Comment 20

[Chris Shoemaker]

Ok, I've made the requested changes.  (I meant to mark all three patches as obsolete by the new ones, but I accidentally forgot to mark the middle one.  I think the original will still apply with fuzz and they are otherwise identical.  I'm not sure if I should mark it obsolete now though.)

How does this work now?  Do I request review again on the new patches, or is the old review good enough to request a checkin?

Comment 21

[Boris Zbarsky [:bzbarsky]]

That creates one copy of the module for every file that includes the header.  You probably want to declare the module extern in the header, then have a non-static module in one of the C++ files...

Did anything actually change in the second patch?  That was the one that got sr+, right?

For the last patch... the indents look odd.  Are there tabs or something in there?  Please use spaces for indentation.

In general, you do want to obsolete older patches, yes.  And the old review is good enough unless the other reviewer asks for substantive changes, which I haven't.

Also, you could just attach a single patch with all the changes if you want.  It'd be easier on my end for sure.  ;)

Comment 22

[Chris Shoemaker]

(In reply to comment #21)
> That creates one copy of the module for every file that includes the header. 
> You probably want to declare the module extern in the header, then have a
> non-static module in one of the C++ files...

will do.

> Did anything actually change in the second patch?  That was the one that got
> sr+, right?

correct.

> For the last patch... the indents look odd.  Are there tabs or something in
> there?  Please use spaces for indentation.

oops.  I was using a foreign emacs.  Fixed up now.

> In general, you do want to obsolete older patches, yes.  And the old review is
> good enough unless the other reviewer asks for substantive changes, which I
> haven't.

ok

> Also, you could just attach a single patch with all the changes if you want. 
> It'd be easier on my end for sure.  ;)

ok, one consolidated patch coming up.

Just for reference: the residual diff between this new patch and the old series, showing just the change to the placement of the logging module, minus the whitespace fix:

Index: mozilla/extensions/jssh/nsJSSh.cpp
===================================================================
--- mozilla.orig/extensions/jssh/nsJSSh.cpp
+++ mozilla/extensions/jssh/nsJSSh.cpp
@@ -51,6 +51,10 @@
 #include "nsMemory.h"
 #include "nsAutoPtr.h"
 
+#ifdef PR_LOGGING
+PRLogModuleInfo *gJSShLog = PR_NewLogModule("jssh");
+#endif
+
 //**********************************************************************
 // Javascript Environment
 
Index: mozilla/extensions/jssh/nsJSSh.h
===================================================================
--- mozilla.orig/extensions/jssh/nsJSSh.h
+++ mozilla/extensions/jssh/nsJSSh.h
@@ -58,8 +58,8 @@
 
 // NSPR_LOG_MODULES=jssh
 #ifdef PR_LOGGING
-static PRLogModuleInfo *sJSShLog = PR_NewLogModule("jssh");
-#define LOG(args) PR_LOG(sJSShLog, PR_LOG_DEBUG, args)
+extern PRLogModuleInfo *gJSShLog;
+#define LOG(args) PR_LOG(gJSShLog, PR_LOG_DEBUG, args)
 #else
 #define LOG(args)
 #endif
Index: mozilla/extensions/jssh/nsJSShServer.cpp
===================================================================
--- mozilla.orig/extensions/jssh/nsJSShServer.cpp
+++ mozilla/extensions/jssh/nsJSShServer.cpp
@@ -46,6 +46,10 @@
 #include "nsJSSh.h"
 #include "nsComponentManagerUtils.h"
 
+#ifdef PR_LOGGING
+PRLogModuleInfo *gJSShLog;
+#endif
+
 //**********************************************************************
 // ConnectionListener helper class
 

Comment 23

[Chris Shoemaker]

Attachment: consolidated patch for jssh bugs

I've fixed the whitespace damage, and created the logging module only once.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273057 [details] [diff] [review]
consolidated patch for jssh bugs

>Index: mozilla/extensions/jssh/nsJSShServer.cpp
>+#ifdef PR_LOGGING
>+PRLogModuleInfo *gJSShLog;
>+#endif

You don't actually want this part.  The nsJSSh.h include already pulled in the extern decl...  Landed without that.

One other question.  Logging the first N bytes of buffer.get() every time through that loop seems pretty odd.  Why do that?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Checked in.  Thanks for the patch!