224797 - nsJARURI should implement nsIURL

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

From Darin's mail (the quoted part I wrote, the non-quoted part Darin wrote):

----------------------------------------------------------
>Oh, I see what the problem is... if I load
>'chrome://global/locale/about.xhtml#test' in the browser, the URI that the
>content sink sees is
>'jar:resource:///chrome/en-US.jar!/locale/en-US/global/about.xhtml#test'
>
>And jar: URIs are not URLs, hence have no refs....

well, i think the solution to this bug might be to make nsJARURI support
nsIURL.  the main reason is that it is valid to use jar: URLs to load
entire websites.  so, unless we fix this bug for jar: URLs, then we will
not be able to support named anchors when a website is loaded from a
jar: URL.

seems to me that the jar: URL could support all attributes on nsIURL
pretty easily.  in the example you gave:

filePath      = "/locale/en-US/global/about.xhtml"
param         = ""
query         = ""
ref           = "test"
directory     = "/locale/en-US/global/"
fileName      = "about.xhtml"
fileBaseName  = "about"
fileExtension = "xhtml"

supporting getCommonBaseSpec and getRelativeSpec should be
straightforward.  looks like nsJARURI::Resolve already takes #ref into
account.
----------------------------------------------------------

There is the question of how the JAR location should be exposed by the URI, if
at all... but this seems like a good idea to me.  This would allow us to remove
some hacks in both the XML and HTML content sinks, and maybe even in the docshell.

Comment 1

[Benjamin Smedberg]

oooh, please! This would save oodles of code in the chrome registry as well.

In relation to this bug, is there a way to do relative links from a JAR uri to
it's base uri, for example:

jar:http://www.mycompany.com/myjarredxulapp.jar!window.xul
wants to link to
http://www.mycompany.com/pdfs/1.pdf

Is this even needed/possible? Would something like "!/pdfs/1.pdf" make sense?

--BDS

Comment 2

[Darin Fisher]

tentatively targeting 1.6 beta... probably more like 1.7 to be honest.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> There is the question of how the JAR location should be exposed by the URI, if
> at all...

Silly me.  That's covered by nsIJARURI.

Could we possibly store the mJAREntry as a nsIURI (eg a file:// url or some
other incarnation of nsStandardURL)?  That would allow us to avoid duplicating
all sorts of code (parsing out filenames, filebasenames, extensions,
directories, etc).  There should be no difference between a JAR filepath and a
file:// url with no host/user/password.  No one but the JAR URI would know of
the existence of this file:// url, so the fact that it QIs to random stuff would
not be an issue...

darin?  What do you think?  Or should the JAR uri just bite the bullet and use
the URLParsers etc by hand on the mJAREntry the way nsStandardURL does on its spec).

This is sort of blocking the de-stringification of XBL's URI handling, so if I
get some time to code I'm willing to implement the approach I described above...
I don't think I grok the URL code well enough to write a full (correct; that's
key; the current JAR impl already has a few XXX comments I just added when
skimming it) nsIURL impl easily...

Comment 4

[Darin Fisher]

bz: your idea of wrapping a nsStandardURL object for handling the path sounds
great to me.  remember: you can call |operator new| from the jar protocol
handler ;-)

Comment 5

[Boris Zbarsky [:bzbarsky]]

As in, I can just new an nsStandardURL?  I think that would belong more in
nsJARURI::SetSpec than in the protocol handler...

One drawback with the approach of using an nsStandardURL is that we need to make
up a bogus scheme for that URL to have (like -moz-jar-internal:///path/to/file).
Would that be acceptable to you?  No one would know about this scheme except the
guts of the JARURI code.

Comment 6

[Darin Fisher]

why bother with a fancy internal scheme?  how about "x:///path/to/file"?  if
done correctly, this scheme should never leak out.  minimize length of string to
minimize bytes copied.

btw, it is illegal for an URI scheme to begin with a '-' character.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Ah, did not know that.  Yes, I can just use x://.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this, maybe

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135375 [details] [diff] [review]
Something like this, maybe

andreas, darin, what do you think?

Comment 10

[Boris Zbarsky [:bzbarsky]]

This patch actually introduces one behavior change... Without this patch, if I
init a jar URI with the spec

  jar:resource:/chrome/classic.jar!/skin/classic/global/

it claims its spec is

  jar:resource:///chrome/classic.jar!/skin/classic/global

while with the patch it claims

  jar:resource:///chrome/classic.jar!/skin/classic/global/

and similarly, relative URI resolution now acts a little differently for URIs
like this (I believe what we had was buggy).

Comment 11

[Darin Fisher]

Comment on attachment 135375 [details] [diff] [review]
Something like this, maybe

>Index: netwerk/protocol/jar/src/nsJARURI.h
>Index: netwerk/protocol/jar/src/nsJARURI.cpp

>+nsJARURI::CreateEntryURL(const nsACString& entryFilename,
...
>+    nsresult rv = stdURL->Init(nsIStandardURL::URLTYPE_NO_AUTHORITY, -1,
>+                               NS_BOGUS_ENTRY_SCHEME + entryFilename,
>+                               charset, nsnull);

passing a dependent concatenation through a XPCOM interface should be avoided
until bug 128288 is fixed (which cannot be fixed without breaking our ABI).
basically, if you cast a dependent concatenation to nsACString&, then that
nsACString& had better not be used in any other dependent concatenation.  you
don't know what nsStandardURL::Init is going to do with your dependent
concatenation ref :-(

did i mention that strings suck?

of course, in this case, we know what nsStandardURL::Init is going to do.
moreover, we know that it is probably not going to take the chance of adding
aSpec (its input argument) to some dependent concatenation ;-)

also, i think you are relying on nsStandardURL to normalize file:////blah
to file:///blah on non-windows platforms.  i think that's ok.


>+    nsCAutoString ourJAREntry;
>+    rv = GetJAREntry(ourJAREntry);
>+    if (NS_FAILED(rv)) return rv;
>+    
>+    *result = (strcmp(ourJAREntry.get(), otherJAREntry.get()) == 0);
>     return NS_OK;

minor codesighs nit (since nsCAutoString has a virtual dtor):

    nsCAutoString ourJAREntry;
    rv = GetJAREntry(ourJAREntry);
    if (NS_SUCCEEDED(rv))    
	*result = (strcmp(ourJAREntry.get(), otherJAREntry.get()) == 0);
    return rv;

that way you avoid duplicating code for the destructor call at each
return statement.


> NS_IMETHODIMP
> nsJARURI::Clone(nsIURI **result)
> {
>     nsresult rv;
> 
>     nsCOMPtr<nsIURI> newJARFile;
>     rv = mJARFile->Clone(getter_AddRefs(newJARFile));
>     if (NS_FAILED(rv)) return rv;
> 
>+    nsCOMPtr<nsIURI> newJAREntryURI;
>+    rv = mJAREntry->Clone(getter_AddRefs(newJAREntryURI));
>+    if (NS_FAILED(rv)) return rv;
>+
>+    nsCOMPtr<nsIURL> newJAREntry(do_QueryInterface(newJAREntryURI));
>+    NS_ASSERTION(newJAREntry, "This had better QI to nsIURL!");
>+    
>     nsJARURI* uri = new nsJARURI();
>+    if (!uri)
>         return NS_ERROR_OUT_OF_MEMORY;
> 
>     NS_ADDREF(uri);
>     uri->mJARFile = newJARFile;
>+    uri->mJAREntry = newJAREntry;
>     *result = uri;
> 
>     return NS_OK;
> }

same minor codesighs nit applies here too.  if you make all the return
statements "return rv", then the compiler can group all the destructors
together and avoid duplicating them.  so, maybe:

   nsJARURI *uri = new nsJARURI();
   if (!uri)
       rv = NS_ERROR_OUT_OF_MEMORY;
   else {
       ...
   }
   return rv;


>+NS_IMETHODIMP
>+nsJARURI::GetParam(nsACString& param)
>+{
>+    return mJAREntry->GetParam(param);
>+}
>+
>+NS_IMETHODIMP
>+nsJARURI::SetParam(const nsACString& param)
>+{
>+    return mJAREntry->SetParam(param);
>+}
>+
>+NS_IMETHODIMP
>+nsJARURI::GetQuery(nsACString& query)
>+{
>+    return mJAREntry->GetQuery(query);
>+}
>+
>+NS_IMETHODIMP
>+nsJARURI::SetQuery(const nsACString& query)
>+{
>+    return mJAREntry->SetQuery(query);
>+}

i'm not sure queries and params make sense.  the jar entry has to
correspond to a file path.  maybe these methods should just fail.


...
>+    nsCOMPtr<nsIURL> url;
>+    rv = CreateEntryURL(otherEntry, otherCharset.get(), getter_AddRefs(url));
>     if (NS_FAILED(rv)) return rv;
> 
>+    nsCAutoString common;
>+    rv = mJAREntry->GetCommonBaseSpec(url, common);
>+    if (NS_FAILED(rv)) return rv;
>+
>+    return FormatSpec(common, commonSpec);
>+}

codesighs nit:

  rv = FormatSpec(...);
  return rv;


>+NS_IMETHODIMP
>+nsJARURI::GetRelativeSpec(nsIURI* uriToCompare, nsACString& relativeSpec)
>+{
...
>+    return NS_OK;
> }

lot's of virtual destructor calls in this function... maybe try to
always return just rv, even on success ;)


overall, the patch looks really good!  sr= with nits picked.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Also, i think you are relying on nsStandardURL to normalize file:////blah

Not at all.  The "entryFileName" string never starts with a '/' except when
someone passes such a beast to SetJAREntry() explicitly; if they do, they should
be shot (I can add an assert to that effect).

> i'm not sure queries and params make sense.

I'm not either, but the old code supports them....  I bet there are people
relying on it.  :(

I'll pick the codesighs nits sometime in the near future, hopefully.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Nevermind.  No one can be relying on GetQuery and GetParam because they've never
worked before... I'm on crack.  ;)

Should those methods really fail, though?  Or just return an empty string?

Question on codesize, by the way.  What optimization level did you use when you
did your tests on that?  Does using -Os make gcc actually sanely consolidate the
destructor invocations on return?

Anyway, I guess I should take this.  ;)

Comment 14

[Darin Fisher]

bz: i did't find -Os (or -O, -O2, -O3) to help in that case.  the same problem
incidentally occurs when you enable exception handling in g++.  all of the
destructor calls get replicated in the exception handling glue that is added by
the compiler.  i noticed that you can have early returns if all the return
statements look the same.  MSVC behaves the same way.  some of those simple
functions in this patch can be reduced in size by nearly 30% just by reworking
the return statements!

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, that works.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix nits

Comment 17

[Darin Fisher]

Comment on attachment 135418 [details] [diff] [review]
Fix nits

nice work!  sr=darin

Comment 18

[Andreas Otte]

Comment on attachment 135418 [details] [diff] [review]
Fix nits

looks good to me too ...

Comment 19

[Benjamin Smedberg]

Quick question which might be incidental to this bug... is it possible to escape
the ! in a JAR uri?

--BDS

Comment 20

[Boris Zbarsky [:bzbarsky]]

What do you mean by "escape"?

Comment 21

[Andreas Otte]

The ! is not escaped by any urlparser but you are free to do it yourself in your
url and this way trick the parser.
 

Comment 22

[Benjamin Smedberg]

Let's imagine some other uri that contains a ! character:

http://www.mozilla.org/this!is!a!stupid!url.jar

would it be possible to create a JAR uri to reference the above URI?

jar:http://www.mozilla.org/this!oops

--bds

Comment 23

[Boris Zbarsky [:bzbarsky]]

Well, first of all the jar code looks for a "!/", not a '!'... ;)

But yes, you would need to escape a "!/" in the filepath if it occurred before
passing the URL string to NewURI.  Alternately, you could create the JAR URI
with an empty string for the filepath and then call SetJAREntry (or SetFilePath
with this patch, I think).

Comment 24

[Boris Zbarsky [:bzbarsky]]

Checked in.  No noticeable perf impact, about 3k size increase, but I will
likely get some of that back in bug 225910 and bug 224765, not to mention
whatever simplifications bsmedberg thinks he can make to the chrome registry.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually it looks like this cut btek Tp by 5ms or so.

Comment 26

[Boris Zbarsky [:bzbarsky]]

It's odd if it did -- I would expect this to have effect on Ts and maybe Txul
far more than on Tp...