372050 - Expose UnescapeString through nsINetUtil

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Prasad Sunkari [:prasad]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1) Gecko/20061205 Iceweasel/2.0.0.1 (Debian-2.0.0.1+dfsg-2)
Build Identifier: Trunk


1. The enumerations of BitMask, which are supposed to be same as defined in nsEscape, are actually different in the Trunk.  nsEscape defines url_Path as PR_BIT(2), which is 4 - but in nsINetUtil.idl, ESCAPE_URL_PATH is defined as 3.  This could lead to a bugs in other parts of the code.

2. Code in the frozen linkage should not use nsEscape and the currently accepted alternative is nsINetUtil (as of today only exposes EscapeString).  The attached patch adds UnescapeString to nsINetUtil.  Also removes the dependency on nsEscape (someday this code will have to migrate to frozen linkage too!)

Note: Bug 350932 addresses an issue similar to (2).  It exposes NS_EscapeURL through nsINetUtil.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Prasad Sunkari [:prasad]]

Attachment: Corrects ESCAPE_URL_PATH, adds UnescapeString, copies code for EscapeString from nsIEscape

Comment 2

[Prasad Sunkari [:prasad]]

Attachment: Corrects ESCAPE_URL_PATH, adds UnescapeString and calls functions from nsEscape (does not copy code)

Comment 3

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

Comment on attachment 256742 [details] [diff] [review]
Corrects ESCAPE_URL_PATH, adds UnescapeString and calls functions from nsEscape (does not copy code)

nsINetUtil needs a new IID

+    char *str = nsCRT::strdup(((nsCString)aString).get());

this should be:
  char* str = ToNewCString(aString);

and it needs an OOM check before calling nsUnescape.

+    if( !result )

coding style here is:
  if (!result)

Comment 5

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

Comment on attachment 256742 [details] [diff] [review]
Corrects ESCAPE_URL_PATH, adds UnescapeString and calls functions from nsEscape (does not copy code)

also, you need to free "str" (with NS_Free if you use ToNewCString)

Comment 6

[Prasad Sunkari [:prasad]]

While changes in coding style and use of ToNewCString can be corrected, I did not understand the need for new IID.  Can someone please explain.

Comment 7

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

It's necessary for binary compatibility. If C++ code uses an interface, it expects the IID to identify it exactly. So, if code uses the new version, and the QueryInterface succeeds, it expects that it can call the new function. But if it's running in an old version of Gecko, the interface does not have the function. If the IID is changed, the QI will fail and the code can avoid the crash.

Comment 8

[Prasad Sunkari [:prasad]]

Attachment: Exposes UnescapeString through nsINetUtil and corrects a minor mask error in existing code.

Corrected the indentation, changed the IID and corrected the creation of new CString.

Comment 9

[Prasad Sunkari [:prasad]]

any progress on this bug???

Comment 10

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

Comment on attachment 258629 [details] [diff] [review]
Exposes UnescapeString through nsINetUtil and corrects a minor mask error in existing code.

- you need to nullcheck _before_ calling nsUnescape
- please add a space between the if and the (

(I mentioned both of this before)

Oh, I didn't notice this before: return NS_ERROR_OUT_OF_MEMORY in the failure case and NS_OK in the success case - this method doesn't return a boolean.

Comment 11

[Prasad Sunkari [:prasad]]

Attachment: Exposes UnescapeString thorough nsINetUtil and corrects a minor mask error in existing code

Corrected the three mistakes pointed out by Biesinger.

1. Null checking the string before calling nsUnescape
2. Indentation style correction (for if statement)
3. Return values (now returns NS_ERROR_OUT_OF_MEMORY and NS_OK)

Sorry for messing up in the previous versions of the patch!

Comment 12

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

Comment on attachment 262095 [details] [diff] [review]
Exposes UnescapeString thorough nsINetUtil and corrects a minor mask error in existing code

There's no point in checking the return value of nsUnescape. It just returns what you pass in.

Also, please move the declaration of result to where you use it:
  char* result = nsUnescape(str);

(or remove it and just use str; I don't care about this either way)

Comment 13

[Prasad Sunkari [:prasad]]

Attachment: Exposes UnescapeString thorough nsINetUtil and a BugFix

Fixed the errors, and the patch is hopefully fine now.

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #13)

Prasad, I'll be happy to check this in for you if you'd like.

Comment 15

[Prasad Sunkari [:prasad]]

(In reply to comment #14)
> (In reply to comment #13)
> 
> Prasad, I'll be happy to check this in for you if you'd like.
>

Ben, It would be great if you can do that :)
and I guess that the status of should be changed FIXED.

Comment 16

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

the status should be changed after the patch has been checked in.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed on trunk. Thanks Prasad!

Comment 18

[Jeff Walden [:Waldo]]

Prasad, would you have time to write a quick test for this?  Here are instructions on how to do so:

http://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests

You can also refer to the following tests to see how things are done:

http://lxr.mozilla.org/mozilla/source/netwerk/test/unit/

The tests don't have to be ridiculously complicated -- just try a few URLs with encoded special characters, one or two that don't contain any percent-encoded characters, and a few with invalid percent encodings (e.g. "http://foo/bar/ba%2z") and check that you get the expected result for each.

Thanks!

Comment 19

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

Attachment: unit test

requesting review since this also fixes some IDL comments

also tests the unescape parts of bug 350932

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 279117 [details] [diff] [review]
unit test

Looks good.

Comment 21

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

Checking in netwerk/base/public/nsINetUtil.idl;
/cvsroot/mozilla/netwerk/base/public/nsINetUtil.idl,v  <--  nsINetUtil.idl
new revision: 1.8; previous revision: 1.7
done
RCS file: /cvsroot/mozilla/netwerk/test/unit/test_unescapestring.js,v
done
Checking in netwerk/test/unit/test_unescapestring.js;
/cvsroot/mozilla/netwerk/test/unit/test_unescapestring.js,v  <--  test_unescapestring.js
initial revision: 1.1
done
Checking in xpcom/io/nsEscape.h;
/cvsroot/mozilla/xpcom/io/nsEscape.h,v  <--  nsEscape.h
new revision: 1.30; previous revision: 1.29
done