126432 - docshell implements its own scheme and anchor parsing

Status: NEW

(Core :: DOM: Navigation, task)

Description

[timeless]

spun off from bug 107575.

Networking has a perfectly good URI parser which does scheme and anchor parsing, docshell should use it.

Comment 1

[Alec Flett]

though you should wait until the latest patch in bug 107676 has landed (I'll
post here when I've landed it) because it cleans up other parts of docshell.

Comment 2

[Adam Lock]

Docshell should be using nsIURI::SchemeIs instead of calling GetScheme and
strcmp'ing it.

Anchor code could probably be replaced by calling nsIURL::GetRef.

Comment 3

[Adam Lock]

Attachment: Patch makes use of SchemeIs

Patch replaces some calls to GetScheme with calls to SchemeIs. Removing the
anchor code will be much more tricky. Ignore the reformatting, I've corrected
some duff indentation.

Reviews?

Comment 4

[Alec Flett]

Most everything looks great..so my one question is, is there a better way to
specify that mail/news/imap are "special"? It seems bad to bury this logic in
this function.. don't we have a list of these type of "special" schemes
somewhere already?

Comment 5

[jag (Peter Annema)]

Comment on attachment 71334 [details] [diff] [review]
Patch makes use of SchemeIs

>Index: mozilla/docshell/base/nsDocShell.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v
>retrieving revision 1.424
>diff -u -b -r1.424 nsDocShell.cpp
>--- mozilla/docshell/base/nsDocShell.cpp	21 Feb 2002 23:16:57 -0000	1.424
>+++ mozilla/docshell/base/nsDocShell.cpp	25 Feb 2002 20:24:55 -0000
>@@ -756,7 +765,9 @@
>     rv = aOriginURI->GetSpec(getter_Copies(originSpec));
>     NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && originSpec, PR_TRUE);
> 
>-    return (! PL_strcmp(targetSpec, originSpec)); // True if full spec is same, false otherwise
>+            // True if full spec is same, false otherwise
>+            return (! PL_strcmp(targetSpec, originSpec));

Since both targetSpec and originSpec are nsXPIDLCString, you can just use
.Equals (since you're touching it).
>@@ -5198,20 +5208,15 @@
>     // should just do a spec compare, rather than two gets of the scheme and
>     // then the path.  -Gagan
>     nsresult rv;
>+    PRBool isAbout = PR_FALSE;
>+    NS_ENSURE_SUCCESS(aURI->SchemeIs("about", &isAbout), PR_FALSE);
>+    if (isAbout) {
>     nsXPIDLCString buffer;
>-    nsCAutoString schemeStr;
>-
>-    rv = aURI->GetScheme(getter_Copies(buffer));
>-    if (NS_FAILED(rv))
>-        return PR_FALSE;
>-
>-    schemeStr = buffer;
>-    if (schemeStr.Equals("about")) {
>         rv = aURI->GetPath(getter_Copies(buffer));
>         if (NS_FAILED(rv))
>             return PR_FALSE;
> 
>-        schemeStr = buffer;
>+        nsCAutoString schemeStr(buffer);

Since schemeStr is a nsXPIDLCString you can call Equals directly on it.

>         if (schemeStr.Equals("blank")) {
>             return PR_FALSE;
>         }

Comment 6

[Adam Lock]

Mitch can you address Alec's concerns about these things being hardcoded?

Comment 7

[Adam Lock]

Attachment: New patch uses .Equals

New patch uses Equals method. Ignore indent problems.

This whole function seems like it should be shifted somewhere else, e.g.
security policy object, but that would be another bug I think.

Reviews?

Comment 8

[Mitchell Stoltz (not reading bugmail)]

Unfortunately, we don't have a standardized way for schemes to advertise that
they are "special," securitywise. We do this check for mail/imap/news schemes in
a number of places. There has been talk about building this information into
URLs or protocol handlers so they can be interrogated about their security
requirements, but for the time being, it's OK to hard-code these checks here.

Comment 9

[Alec Flett]

Comment on attachment 71521 [details] [diff] [review]
New patch uses .Equals

ok, if mitch is cool with it, then so am I.

Adam, instead of the 
nsCAutoString schemeStr(buffer);
stuff, you can say
if (nsDependentCString(buffer).Equals("blank"))
and avoid the extra copy.

Comment 10

[Kathleen :Brade]

Comment on attachment 71521 [details] [diff] [review]
New patch uses .Equals

* initialization of mValidOrigin should happen in order of declaration (before
mIsBeingDestroyed)

* I know you didn't change this but is it really ok to return true for file
urls?  What if the urls were on completely different volumes?

* Can't you use aTargetURI->Equals(aOriginURI, &isSame) instead of doing the
string stuff?

* there may be a "piece" comparison utility in io services but I'm not sure
that is available to docshell (?)

r=brade with initialization fix/change and addressing uri->Equals

Comment 11

[jag (Peter Annema)]

Alec: no need for the nsDependentCString. |buffer| is of type |nsXPIDLCString|.

Comment 12

[Adam Lock]

Attachment: New patch

Patch incorporates suggestions, reorders initialization of mValidateOrigin &
removes redundant string assignment for comparison op.

Comment 13

[Adam Lock]

Attachment: Updated patch

Carrying r= forward. This patch is an update following recent
nsXPIDLCString->nsCAutoString changes

Comment 14

[Darin Fisher]

adam: you should really be checking the return value of SchemeIs.  we don't have
control over all nsIURI impl's and shouldn't assume that they all do the right
thing.

Comment 15

[Adam Lock]

1.0.1

Will try and check the return codes, but it's pretty hard to tell what this
method should do in the event of failure. Some error handling in this code
return PR_TRUE (i.e. yes security check has passed) and some returns PR_FALSE.

Mitch, would I be right to change this code to return PR_FALSE for all genuine
errors?

Comment 16

[Adam Lock]

Retarget 1.1beta

Comment 17

[Juan José Mata]

Moving out to 'Future' since 'mozilla1.1beta' is past us.

Comment 18

[O. Atsushi (Torisugari)]

About anchor parsing, docshell is doing a delicate things and this is an INVALID bug, in my opinion.

About scheme parsing, GetScheme(...) remains 4, and 3 of the 4 is reasonable. What we should fix seems only in nsDocShell::ShouldAddToSessionHistory(nsIURI * aURI).
http://hg.mozilla.org/mozilla-central/annotate/cc552c8892cc/docshell/base/nsDocShell.cpp#l9813

And probably we should use NS_IsAboutBlank(...) rather than SchemeIs(...), instead of GetScheme(...).