278262 - JAR URIs should resolve relative URIs in the base section

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[Benjamin Smedberg]

When dealing with bug 246209 and my semi-annual chrome registry whacking, I
found that JAR URIs cannot contain a relative path component in the "first" part
of the URI: i.e.

jar:../relative/path.jar!/path/inside

This isn't that hard to fix, and it makes life a lot less hectic for the chrome
registry. Patch forthcoming.

Comment 1

[Benjamin Smedberg]

Attachment: semi-relative JAR URIs, rev. 1

Comment 2

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

Comment on attachment 171170 [details] [diff] [review]
semi-relative JAR URIs, rev. 1

sharing the string parsing code with nsJARURI::SetSpec instead of duplicating
it would be nice...

Comment 3

[Benjamin Smedberg]

Comment on attachment 171170 [details] [diff] [review]
semi-relative JAR URIs, rev. 1

biesi pointed out that nsJARURI::SetSpec has very similar logic, and to avoid
codesize I should try to factor them together. nsJARURI::SetSpecWithBase
forthcoming.

Comment 4

[Benjamin Smedberg]

Attachment: semi-relative JAR URIs, rev. 2

This patch isn't quite as small as I'd like, but it duplicates less code than
the last one, and it reduces string<->ns*URI round-trips several times during
the parsing of a relative URI reference.

Comment 5

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

sorry for the delay, I didn't have any time for reviewing the patch this
weekend, I'll try to get to it tuesday or wednesday

Comment 6

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

Comment on attachment 171232 [details] [diff] [review]
semi-relative JAR URIs, rev. 2

modules/libjar/nsJARURI.cpp
+nsJARURI::SetSpecWithBase
-    nsCOMPtr<nsIIOService> ioServ(do_GetIOService(&rv));
+    nsCOMPtr<nsIIOService> ioServ (do_GetIOService());

hm, why remove the specific error code in favor of a generic one?

+	 aBaseURL->QueryInterface(NS_GET_IID(nsJARURI),
getter_AddRefs(otherJAR));
+	 if (!otherJAR) return NS_NOINTERFACE;

NS_ERROR_INVALID_ARG maybe? doesn't matter much.

+	 mJAREntry = do_QueryInterface(entry);
+	 if (!mJAREntry)
+	     return NS_NOINTERFACE;

personally I'd only use an assertion here.

+    while (begin != end && *begin != ':')
+	 ++begin;

FindCharInReadable? guess for 4 chars that won't matter :-)

+				delim_end   (end);

hm, why do you need this var?

 nsJARURI::Equals(nsIURI *other, PRBool *result)
 {
+    nsresult rv;

can you move it back down, to where it is first used?

Comment 7

[Benjamin Smedberg]

Comment on attachment 171232 [details] [diff] [review]
semi-relative JAR URIs, rev. 2

I need delim_end because RFindInReadable modifies it's "end" iterator, and I
need the real "end" iterator farther down for a dependent substring.

Comment 8

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

ah, sorry. I was thinking it changed the start iterator.

Comment 9

[Darin Fisher]

Comment on attachment 171232 [details] [diff] [review]
semi-relative JAR URIs, rev. 2

>Index: modules/libjar/nsJARURI.cpp

>+nsresult
>+nsJARURI::SetSpecWithBase(const nsACString &aSpec, nsIURI* aBaseURL)
> {
>     nsresult rv;
>+
>+    nsCOMPtr<nsIIOService> ioServ (do_GetIOService());

nit: remove space between "ioServ" and "("


>+        aBaseURL->QueryInterface(NS_GET_IID(nsJARURI), getter_AddRefs(otherJAR));
>+        if (!otherJAR) return NS_NOINTERFACE;

nit: use NS_ENSURE_TRUE here?  warnings are good :)


>+        nsCOMPtr<nsIStandardURL> entry 
>+            (do_CreateInstance(NS_STANDARDURL_CONTRACTID));

style nit: it all fits on one line, but mainly don't start line with "("

	  nsCOMPtr<nsIStandardURL>
entry(do_CreateInstance(NS_STANDARDURL_CONTRACTID));


>+    if (!scheme.EqualsLiteral("jar"))
>         return NS_ERROR_MALFORMED_URI;

nit: NS_ENSURE_TRUE?  or can this even be a NS_ASSERTION?

Comment 10

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Unit test v1

Unit test v1

Comment 11

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Unit test v2

Unit test v2

* Better than unit test v1.

Comment 12

[Nickolay_Ponomarev]

Checking in test_bug278262.js;
/cvsroot/mozilla/modules/libjar/test/unit/test_bug278262.js,v  <--  test_bug278262.js
initial revision: 1.1
done