109179 - replace nsStdEscape with NS_EscapeURL

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Darin Fisher]

replace nsStdEscape with NS_EscapeURL to avoid buffer copy when escaping is not
necessary.

see:
http://lxr.mozilla.org/seamonkey/search?string=nsStdEscape

for example, the call to nsStdEscape in NS_MakeAbsoluteURLWithCharset should
definitely be converted to using NS_EscapeURL.

this bug depends on bug 103916, which adds NS_EscapeURL.

Comment 1

[Darin Fisher]

-> 0.9.7

Comment 2

[Andreas Otte]

I will also look into this after the patch for bug 103916 lands.

Comment 3

[Darin Fisher]

dp says that nsStdEscape is called 14,134 times at startup (in a 2 week old
build, which wouldn't include the patch for bug 103916).  in that build,
nsStdEscape is .64% of main1, so fixing this bug may give us a significant
performance boost at startup.

Comment 4

[Andreas Otte]

dp should repeat his test with the new builds, we may have got most of the
nsStdEscape usage by the patch in 103916.

Comment 5

[Darin Fisher]

true, but it just goes to show that all instances of nsStdEscape should be
replaced by NS_EscapeURL.

Comment 6

[Darin Fisher]

Attachment: v1.0 patch

this patch has only been tested on linux... i'll test on windows and mac before
and worry about OS/2 when i check in ;)

Comment 7

[Darin Fisher]

Comment on attachment 60576 [details] [diff] [review]
v1.0 patch

forgot to fix up some calls to nsStdUnescape

Comment 8

[Darin Fisher]

-> 0.9.8

Comment 9

[Darin Fisher]

Attachment: v1.1 patch - step 1: revise current URL escaping API

Comment 10

[Darin Fisher]

andreas: can you review this latest patch?  thx!

Comment 11

[Andreas Otte]

Comment on attachment 63946 [details] [diff] [review]
v1.1 patch - step 1: revise current URL escaping API

r=[e-mail]

Maybe it should be made more clear in nsEscape.h that esc_Forced is not a mask
for an url part but a special mask to force escaping.

Comment 12

[Darin Fisher]

agreed... i'll update the patch.

Comment 13

[Alec Flett]

Comment on attachment 63946 [details] [diff] [review]
v1.1 patch - step 1: revise current URL escaping API

I'm a little worried that NS_EscapeURLPart no longer truncates its "result" -
though it seems intentional. Do we think its worth a potentially confusing
semantic change just to optimize the few callers who might want this appending
behavior?

Assuming there are good reasons for all of this (But let's put them in the bug
for good measure)
sr=alecf

A side suggestion: there isn't really an Adopt() for nsAString, but seems like
you're placing an extra burden on the caller to check the resulting value to
determine which output to use....

what if you made a 
NS_Escape(const char *str, PRInt32 len, PRInt16 mask, nsSharableString&
result);

which wrapped the other NS_Escape, and did an Adopt() to take ownership.. I
dunno. CC'ing jag for suggestions

Comment 14

[Alec Flett]

cc'ing jag and dbaron for string usage questions.

Comment 15

[Darin Fisher]

alecf: that API would require that nsCAutoString support sharing, which it
currently does not.  otherwise, we'd suffer a performance loss in the current
world.  and, Adopt takes a |char *| not a |const char *|, so it wouldn't be
useful here.

the API i came up with for NS_EscapeURLPart is meant to be a low-level utility
function.  it doesn't do anymore work then necessary.  i can certainly imagine
layering other functions on top of NS_EscapeURLPart, but i think the function
itself should stay as it is... very basic.

Comment 16

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

Not quite sure exactly what the string questions were (if any are left), but
please spell deprecated correctly. :-)

Comment 17

[Alec Flett]

ok, cool :)
sr=alecf still stands of course

Comment 18

[Darin Fisher]

checked in v1.1 patch (solving the first part of this bug)

Comment 19

[Darin Fisher]

Attachment: patch to replace nsStdEscape w/ NS_EscapeURLPart in nsIOServiceXXX

Comment 20

[Darin Fisher]

Comment on attachment 63946 [details] [diff] [review]
v1.1 patch - step 1: revise current URL escaping API

marking obsolete since this has been checked in.

Comment 21

[Doug Turner (:dougt)]

Comment on attachment 64613 [details] [diff] [review]
patch to replace nsStdEscape w/ NS_EscapeURLPart in nsIOServiceXXX


leave the OS2 commented #defines unless mike thinks that they should go.

r=dougt

Comment 22

[Darin Fisher]

mkaply: any problems with these changes to nsIOServiceOS2.cpp?

Comment 23

[Mike Kaply [:mkaply]]

Looks good to me.

Go ahead and remove the commented out #ifdefs.

Comment 24

[Alec Flett]

Comment on attachment 64613 [details] [diff] [review]
patch to replace nsStdEscape w/ NS_EscapeURLPart in nsIOServiceXXX

sr=alecf

Comment 25

[Darin Fisher]

will land this once the tree opens for mozilla 0.9.9

Comment 26

[Darin Fisher]

so the mac portion of this patch had to be backed out and there was a regression
in the windows/os2 builds as a result of this (see bug 122027).  i've got a
revised patch coming...

Comment 27

[Darin Fisher]

Attachment: patch - file:// should have been file:/// on mac, windows, and OS/2

tested this patch on the mac.  the original patch was backed out on the mac
since it caused startup problems.  i've verified that the new patch works.  was
all due to the fact that file:// should have been file:/// :-/

Comment 28

[Gagan]

Comment on attachment 66824 [details] [diff] [review]
patch - file:// should have been file:/// on mac, windows, and OS/2

r=gagan

Comment 29

[Chris Waterson]

Comment on attachment 66824 [details] [diff] [review]
patch - file:// should have been file:/// on mac, windows, and OS/2

sr=waterson

Comment 30

[Darin Fisher]

patch checked in.

Comment 31

[Charles Manske]

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

Comment 32

[Darin Fisher]

Attachment: final patch

removes last reference to nsStdEscape

Comment 33

[Alec Flett]

Comment on attachment 67603 [details] [diff] [review]
final patch

sr=alecf
nice cleanup too.

Comment 34

[Darin Fisher]

Attachment: final nsStdEscape patch + removes all instances of nsStdUnescape

alecf: thx for the speedy review... i almost forgot that i should also be
replacing nsStdUnescape w/ NS_UnescapeURL :-/

Comment 35

[Darin Fisher]

this latest patch also fixes a potential memory leak in FTP.

Comment 36

[Doug Turner (:dougt)]

Comment on attachment 67628 [details] [diff] [review]
final nsStdEscape patch + removes all instances of nsStdUnescape

is there no auto string that we can use?  

Is your indenting off a bit in nsMsgFolder.cpp?

Comment 37

[Darin Fisher]

nsMsgFolder uses tabs in place of spaces, but not uniformly.  i plan to fix up
the indentation when i check this in.

auto string for what?  if you are referring to the places where i capture a
char* and call nsMemory::Free explicitly, i think there is no appropriate string
class, because the call to NS_UnescapeURL changes the length of the string.  it
inline decodes escape sequences, which always reduces the length of the string.
 it just seems like a hack/overkill to use a string class for something like this.

  nsXPIDLCString foo;
  bar->GetFoo(getter_Copies(foo));
  NS_UnescapeURL((char *) foo.get());
  // now, foo.Length is potentially wrong
  NS_SomeFunction(foo); // watch out!!

i'd worry about abusing string code just to avoid explicitly adding nsMemory::
Free calls to the code ;-)

Comment 38

[Alec Flett]

Comment on attachment 67628 [details] [diff] [review]
final nsStdEscape patch + removes all instances of nsStdUnescape

yeah, const-casting is bad :) but I had a similar question while I was
reviewing - can you put some comments saying something to the effect of "using
raw pointer because we're modifying the string"?
sr=alecf with those patches

Comment 39

[Darin Fisher]

will do.

Comment 40

[Darin Fisher]

marking FIXED :)