bad string usage in nsObjectFrame::Reflow

VERIFIED DUPLICATE of bug 108557

Status

()

Core
Plug-ins
P3
normal
VERIFIED DUPLICATE of bug 108557
17 years ago
16 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
Future
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 3 obsolete attachments)

(Assignee)

Description

17 years ago
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...
(Assignee)

Comment 1

17 years ago
Created attachment 21670 [details] [diff] [review]
first draft of patch - not tested yet
(Assignee)

Comment 2

17 years ago
Created attachment 21671 [details] [diff] [review]
slightly improved
(Assignee)

Comment 3

17 years ago
Oops, I meant to assign this to myself.
Assignee: av → dbaron

Updated

17 years ago
Keywords: approval, patch, review
(Assignee)

Updated

17 years ago
Keywords: approval, patch, review
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla0.8
(Assignee)

Comment 4

17 years ago
Created attachment 22615 [details] [diff] [review]
brought up to date
(Assignee)

Comment 5

17 years ago
Created attachment 22618 [details] [diff] [review]
with a little more whitespace cleanup
(Assignee)

Comment 6

17 years ago
Created attachment 23102 [details] [diff] [review]
with some nsCOMPtr cleanup as well

Comment 7

17 years ago
-        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?
(Assignee)

Comment 8

17 years ago
Created attachment 24533 [details] [diff] [review]
patch incorporating jag's suggestions

Comment 9

17 years ago
r=jag
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.8 → mozilla0.9
(Assignee)

Comment 10

17 years ago
*** Bug 68807 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 11

17 years ago
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

17 years ago
Yeah, that'd be nice.

Comment 13

17 years ago
24553 -- r=av

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

Comment 14

17 years ago
80col?
(Assignee)

Comment 15

17 years ago
Yes, 80 columns.

Comment 16

17 years ago
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
(Assignee)

Comment 17

17 years ago
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).
(Assignee)

Comment 18

17 years ago
Moving to 0.9.1 for dealing with the remaining issues.
Target Milestone: mozilla0.9 → mozilla0.9.1
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.1 → mozilla0.9.2
(Assignee)

Updated

17 years ago
Target Milestone: mozilla0.9.2 → Future

Comment 19

16 years ago
Created attachment 70495 [details] [diff] [review]
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

16 years ago
Created attachment 70496 [details] [diff] [review]
oops, fix second Find too

Comment 21

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

Comment 22

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

Comment 23

16 years ago
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

16 years ago
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...
(Assignee)

Comment 25

16 years ago
Substring doesn't allocate a new buffer.  It's dependent on the existing one.
(Assignee)

Comment 26

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

Comment 27

16 years ago
Created attachment 71471 [details] [diff] [review]
use Substring() instead of Find()

OK, don't want to bother with nsReadableUtils.
Attachment #70495 - Attachment is obsolete: true
Attachment #70496 - Attachment is obsolete: true

Comment 28

16 years ago
Created attachment 71474 [details] [diff] [review]
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) 


?
Attachment #71471 - Attachment is obsolete: true

Comment 29

16 years ago
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

16 years ago
JAVA_CLASS_ID needs to go away as well. See bug 108557.

Comment 31

16 years ago
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

16 years ago
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.
Depends on: 108557

Comment 33

16 years ago
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

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

Comment 35

16 years ago
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
Attachment #71474 - Flags: needs-work+

Comment 36

16 years ago
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

16 years ago
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?
(Assignee)

Comment 38

16 years ago

*** This bug has been marked as a duplicate of 108557 ***
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → DUPLICATE

Comment 39

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

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.