64209 - bad string usage in nsObjectFrame::Reflow

Status: VERIFIED DUPLICATE of bug 108557

(Core Graveyard :: Plug-ins, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There's some bad string usage in nsObjectFrame::Reflow.  There's a free memory
mismatch (FMM) at |delete [] cString|, and also some leaks.  Patch coming
shortly, although it will need a good bit more testing...

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: first draft of patch - not tested yet

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: slightly improved

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oops, I meant to assign this to myself.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: brought up to date

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: with a little more whitespace cleanup

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: with some nsCOMPtr cleanup as well

Comment 7

[Peter "jag" Annema]

-        if((rv = GetBaseURL(baseURL)) != NS_OK)
-            return rv;
+        if (NS_SUCCEEDED(rv = GetBaseURL(*getter_AddRefs(baseURL))))
+          return rv;

Shouldn't that be NS_FAILED ?


-        rv = NS_NewURI(&fullURL, src, baseURL);
-        if ( rv != NS_OK )
+        rv = NS_NewURI(getter_AddRefs(fullURL), src, baseURL);
+        if (NS_SUCCEEDED(rv))

And same there...

+        rv = NS_NewURI(getter_AddRefs(fullURL), src, baseURL);
         if ( rv != NS_OK )

And then you could use NS_FAILED there too...

+        char *extension = (offset == kNotFound)
...
+        if (extension)
+          nsMemory::Free(extension);

Could you use an nsXPIDLCString here?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch incorporating jag's suggestions

Comment 9

[Peter "jag" Annema]

r=jag

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I should probably replace my replacement for mimeType in two places with a

static const char* kJavaMimeType = "application/x-java-vm";

so I don't write it in two places...

Comment 12

[Peter "jag" Annema]

Yeah, that'd be nice.

Comment 13

[av (gone)]

24553 -- r=av

Just out of curiosity: what is your 'comfortable' line length after which you 
break it in two?

Comment 14

[Blake Ross]

80col?

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, 80 columns.

Comment 16

[Chris Waterson]

Should

  if (classid.Find("java:") != -1)

be something like:

  if (Substring(classid, 0, 5) == NS_LITERAL_CSTRING("java:"))

otherwise, you'll match

  xxxjava:etc

and end up with

  va:etc

Same for ``clsid:''. (scc, correct my string-fu if I've botched it.) Certainly, 
you haven't changed the semantics, so feel free to check in as-is. (I just don't 
know if you want to be cvs-blamed for it. ;-)

The rest looks ok.

sr=waterson

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in 2001-03-09 19:17 PST but keeping the bug opened to deal with the
issues waterson mentioned (which were in code that I was re-indenting).

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Moving to 0.9.1 for dealing with the remaining issues.

Comment 19

[Denis Antrushin]

Attachment: classid.Find fix

Why not just to check offset returned by Find?
If it's zero, then we have exactly what we're looking for

Comment 20

[Denis Antrushin]

Attachment: oops, fix second Find too

Comment 21

[timeless]

in theory, a find should be more expensive than a substring+compare

Comment 22

[Denis Antrushin]

This is what last "1" in Find() for...

Comment 23

[jag (Peter Annema)]

Actually, that will cause it to look at offsets 0 and 1. That old code is weird
:-) If you mean "Compare this substring against that string" then why not write
it like that? See waterson's comment for a good example.

Comment 24

[Denis Antrushin]

I wanted it to look only at offset 0, but this is impossible
(should new bug be filed on this?). The ideal case would be to have
StartsWith(). Unfortunately, we don't have it. Substring and compare 
causes extra (and unneeded) allocation...

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Substring doesn't allocate a new buffer.  It's dependent on the existing one.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Then again, I wouldn't mind putting StringBeginsWith and StringEndsWith in
nsReadableUtils...

Comment 27

[Denis Antrushin]

Attachment: use Substring() instead of Find()

OK, don't want to bother with nsReadableUtils.

Comment 28

[Denis Antrushin]

Attachment: use Substring() instead of Find()

Oops, attached wrong diff, sorry (64209 vs. 64092)





BTW, what about 


   nsCRT::strncmp(classid.get() NS_LITERAL_STRING("java:").get(), 5) 


?

Comment 29

[jag (Peter Annema)]

You can only have the compiler concatenate string literals that way, not char
pointers or char arrays. Code looks okay on first glance.

Comment 30

[Peter Lubczynski]

JAVA_CLASS_ID needs to go away as well. See bug 108557.

Comment 31

[Denis Antrushin]

Huh, fix for bug #108557 completely removed these string manipulations.
(it's wrong, though :-) ).
Should we close this bug (and make sure fix for 108557 correctly works with
strings) or make it depend on 108557 ?

jag: I meant to write nsCRT::strncmp(classid.get(), .....); (missed comma). :-)

Comment 32

[jag (Peter Annema)]

It certainly wouldn't hurt to combine those two efforts. Can you post your
comments in there on their patch (based on your patch)? I'll mark it "depends
on" for now.

Comment 33

[Denis Antrushin]

They get rid of second string find ("clsid:") and use
Substring(classid, 0, 5) == NS_LITERAL_STRING(blah-blah) for first one.
Even more, I think they can get rid of classid.Cut() at all...

Comment 34

[Denis Antrushin]

jag, dbaron: I still wonder if substring() and compare is faster/better
that nsCRT::strncmp()?

Comment 35

[Patrick C. Beard]

Comment on attachment 71474 [details] [diff] [review]
use Substring() instead of Find()

Why not simply use nsCRT::strncmp() to do the comparisons, rather than
Substring()?

http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCRT.h#208

Comment 36

[jag (Peter Annema)]

I don't care. Substring doesn't copy, so it should be just as cheap, and I guess

  if (Substring(str1, 0, 5) == NS_LITERAL_STRING("java:"))

reads easier for me than

  if (!nsCRT::strncmp(str1.get(), NS_LITERAL_STRING("java:").get(), 5))

Comment 37

[Denis Antrushin]

Note that bug #108557 is fixed; all we have now is
nsCRT::strncmp(classid.get(), NS_LITERAL_STRING("java:").get(), 5);

Can we close this bug now?

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** This bug has been marked as a duplicate of 108557 ***

Comment 39

[shrirang khanzode]

mass duplicate verifications . For filtering purposes, pls use keywd
"massdupverification"