Closed Bug 372050 Opened 13 years ago Closed 13 years ago

Expose UnescapeString through nsINetUtil

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: prasad, Unassigned)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
Attachment #256737 - Attachment is obsolete: true
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)
Attachment #256742 - Flags: review?(cbiesinger) → review-
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: PC → All
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)
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.
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.
Corrected the indentation, changed the IID and corrected the creation of new CString.
Attachment #256742 - Attachment is obsolete: true
Attachment #258629 - Flags: review?(cbiesinger)
Blocks: 374862
any progress on this bug???
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.
Attachment #258629 - Flags: review?(cbiesinger) → review-
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!
Attachment #258629 - Attachment is obsolete: true
Attachment #262095 - Attachment is patch: true
Attachment #262095 - Attachment mime type: text/x-patch → text/plain
Attachment #262095 - Flags: review?(cbiesinger)
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)
Attachment #262095 - Flags: review?(cbiesinger) → review-
Fixed the errors, and the patch is hopefully fine now.
Attachment #262095 - Attachment is obsolete: true
Attachment #262268 - Flags: review?(cbiesinger)
Attachment #262268 - Flags: superreview+
Attachment #262268 - Flags: review?(cbiesinger)
Attachment #262268 - Flags: review+
(In reply to comment #13)

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

(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.

the status should be changed after the patch has been checked in.
Fixed on trunk. Thanks Prasad!
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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!
Flags: in-testsuite?
Attached patch unit testSplinter Review
requesting review since this also fixes some IDL comments

also tests the unescape parts of bug 350932
Attachment #279117 - Flags: superreview?(bzbarsky)
Attachment #279117 - Flags: review?(bzbarsky)
Comment on attachment 279117 [details] [diff] [review]
unit test

Looks good.
Attachment #279117 - Flags: superreview?(bzbarsky)
Attachment #279117 - Flags: superreview+
Attachment #279117 - Flags: review?(bzbarsky)
Attachment #279117 - Flags: review+
Attachment #279117 - Flags: approval1.9+
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
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.