350932 - expose NS_EscapeURL through nsINetUtil

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[aaronr]

Currently nsINetUtil interface has the function escapeString to expose NS_Escape functionality to components and extensions through nsIOService.  The XForms extension (and possibly other extensions) needs access to NS_EscapeURL, so I'd like to add a function called 'escapeURL' to the nsINetUtil interface and implement the work in nsIOService.  We currently access NS_EscapeURL directly, but that won't be possible once --enable-libxul is on by default for the trunk.

Comment 1

[aaronr]

Attachment: patch

adds interface method that calls directly into NS_EscapeURL

Comment 2

[Darin Fisher]

Comment on attachment 237289 [details] [diff] [review]
patch

The flag values really ought to be defined on this interface as well.  People should not have to #include nsEscape.h in order to get the flag values, or they should not have to write numeric literals in their place.

Comment 3

[aaronr]

(In reply to comment #2)
> (From update of attachment 237289 [details] [diff] [review] [edit])
> The flag values really ought to be defined on this interface as well.  People
> should not have to #include nsEscape.h in order to get the flag values, or they
> should not have to write numeric literals in their place.
> 

My bad, I should have thought of this.  I'm guessing that the best way to do this would be to define the values in the interface and then change the flags in nsEscape.h to be based off of the interface flags.  I'll put together a patch that does this.

Comment 4

[aaronr]

Attachment: patch 2

patch to address darin's comment.

Comment 5

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

Comment on attachment 238554 [details] [diff] [review]
patch 2

biesi is already reviewing a similar change to nsINetUtil... Can we resurrect this one?

Comment 6

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

I'd really rather not expose _two_ functions to escape strings here, especially if it's unclear how they differ...

Comment 7

[Prasad Sunkari [:prasad]]

Attachment: Exposes both EscapeURL and UnescapeURL though nsINetUtil

This patch exposes both UnescapeURL and EscapeURL (originally NS_EscapeURL and NS_UnescapeURL from nsEscape.h) through nsINetUtil.

The main difference between EscapeString and EscapeURL will be that EscapeURL is a more specialized function for %XX escaping URLs. This function recognizes various parts of the URL... scheme, username, password, path, filename etc; and lets the user escape any of the parts selectively.  The output is appended to the result string (unlike EscapeString where the output is copied to result string)

Comment 8

[Prasad Sunkari [:prasad]]

Biesi, more information on why URL escaping is different is discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=61269

Comment 9

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

OK. I accept the need for escapeURL.

Instead of adding unescapeURL, I'd prefer if you just added a flags parameter to the existing unescape function and made it call NS_UnescapeURL.

What I would like to see in a patch that adds it:
- A comment in nsEscape.h as in aaronr's patch
- Comments about the individual flag values as in prasad's patch
- Combining things like ESCAPE_MINIMAL from the individual flags as in aaronr's patch
- Using (1 << N) for the actual values, so that this is closer to the .h file (e.g. as in how nsIChannel.idl defines its flags)


The implementations don't match the XPCOM semantics, you should truncate the string first (or change it to an inout parameter). However, I think this would be easier to use if the signature looked like:

  ACString escapeURL(in ACString string, in unsigned long flags);

And if it always set ESCAPE_ALWAYS_COPY (and didn't add that flag on the interface). What do you think? I'm not sure that the performance optimization of avoiding the copy sometimes is very valuable here.


Also, don't use ToNewCString to implement this. Instead, pass aStr.BeginReading().

Comment 10

[Prasad Sunkari [:prasad]]

Attachment: Expose EscapeURL and update UnescapeString to work for URLs

Biesi, I updated the patch as per your comments.

I am not sure if what I did is correct, specially w.r.t truncating the string.  Can you explain me the need from truncating?

Comment 11

[Prasad Sunkari [:prasad]]

Can Darin or someone else review the patch if Biesi is busy.

Comment 12

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

Comment on attachment 268061 [details] [diff] [review]
Expose EscapeURL and update UnescapeString to work for URLs

nsINetUtil.idl
+  /** Copy input to result buffer even if escaping is unnecessary */
+  // Did not see situations where NS_EscapeURL is called without esc_AlwaysCopy
+  // const unsigned long ESCAPE_URL_ALWAYS_COPY = 1 << 13;

please remove that

+   * @param aFlags only ESCAPE_URL_ONLY_NONASCII, ESCAPE_URL_SKIP_CONTROL and 
+   *               ESCAPE_URL_ALWAYS_COPY are recognized

perhaps mention that 0 is a valid value too

+%{C++

please don't add C++ blocks to necko IDL files... just update the callers instead

nsIOService.cpp
+  PRBool escaped = NS_EscapeURL(aStr.BeginReading(), aStr.Length(), aFlags, aResult);

You should add esc_AlwaysCopy to the flags here too

With that flag, the scape/unescape functions always return true, so you can just always return NS_OK

r=biesi with those changes. please attach a patch with those changes.

Comment 13

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

(In reply to comment #10)
> I am not sure if what I did is correct, specially w.r.t truncating the string. 
> Can you explain me the need from truncating?

Yes, it's correct. Thanks.

The need is that XPCOM functions shouldn't rely on any specific value of out parameters and thus overwrite the values there.

Comment 14

[Prasad Sunkari [:prasad]]

Attachment: Exposes both EscapeURL and UnescapeURL though nsINetUtil - Updated as per Biesi's comments.

Updated the patch according to Biesi's comments above.

Comment 15

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

Comment on attachment 268732 [details] [diff] [review]
Exposes both EscapeURL and UnescapeURL though nsINetUtil - Updated as per Biesi's comments.

oh... sorry, I missed this in my previous review:

please move escapeString and its flags after toImmutableURI, so that the escape functions are all grouped together
+   * %XX-Escape invalid chars in a URL segment. Has no effect if the
+   * URL segment is already escaped. Otherwise, the escaped URL segment is
+   * appended to |aResult|

this description is wrong. it should be something like:

+   * %XX-Escape invalid chars in a URL segment.
+   *
+   * @param aStr the URL to be escaped
+   * @param aFlags the URL segment type flags
+   *
+   * @return the escaped string (the string itself if escaping did not happen)

+   * @param aFlags only ESCAPE_URL_ONLY_NONASCII, ESCAPE_URL_SKIP_CONTROL and 
+   *               ESCAPE_URL_ALWAYS_COPY are recognized.  If |aFlags| is 0
+   *               the function act as nsEscape::Unescape

ESCAPE_URL_ALWAYS_COPY doesn't exist anymore, so you shouldn't mention it here :)

nsEscape is not a class... just write that if flags is 0, all escape sequences are unescaped.

Comment 16

[Prasad Sunkari [:prasad]]

Attachment: Expose EscapeURL and update UnescapeString to work for URLs

Updated as per the comments above.

Comment 17

[Prasad Sunkari [:prasad]]

biesi, can you check this in for me... I don't have commit access to CVS.

Comment 18

[Nickolay_Ponomarev]

mozilla/xpcom/io/nsEscape.h                 1.29
mozilla/netwerk/base/public/nsINetUtil.idl  1.6
mozilla/netwerk/base/src/nsIOService.cpp    1.197

Comment 19

[Nickolay_Ponomarev]

Comment on attachment 238554 [details] [diff] [review]
patch 2

The newer patches obsoleted this patch, right?

Comment 20

[Jeff Walden [:Waldo]]

Prasad: think you'd have time to write some brief testcases for this?  See the following URL for details:

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

Comment 21

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

unescape testcase added:
mozilla/netwerk/test/unit/test_unescapestring.js 	1.1