163225 - success or failure of ExtractScheme is not enough to decide if a url is absolute or relative

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Andreas Otte]

Success or failure of ExtractScheme is not enough to decide if an url is
absolute or relative after checkin of patch of bug 32966

Comment 1

[Andreas Otte]

Current NewURI functions use ExtractScheme to decide if an url is absolute or
relative. It is not so easy anymore ... leave that decision to ::Resolve.

Comment 2

[Andreas Otte]

Attachment: patch to set aBaseURI not to null if scheme detected

Comment 3

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

+    if (!NS_SUCCEEDED(rv)) {

use if (NS_FAILED(rv))

Comment 4

[Andreas Otte]

Attachment: better patch

incorporating cbiesingers suggestion

Comment 5

[Andreas Otte]

Attachment: better performing patch

this version checks in nsStandardURL::Init before calling resolve to reduce the
costs of removing the check in nsIOService.cpp. Thanks Darin!

Comment 6

[Andreas Otte]

Bradley, this is a followup to bug 32966, want to r=?

Comment 7

[Bradley Baetz (:bbaetz)]

Comment on attachment 96687 [details] [diff] [review]
better performing patch


> >+    if (baseURI) {
>+        PRUint32 start, end;
>+        // pull out the scheme and where it ends
>+        nsresult rv = ExtractURLScheme(spec, &start, &end, nsnull);
>+        if (NS_SUCCEEDED(rv)) {
>+            nsACString::const_iterator slash;
>+            spec.BeginReading(slash);
>+            slash.advance(end);
>+            // then check if // follows
>+            // if it follows aSpec is really absolute ... 
>+            // ignore aBaseURI in this case
>+            if (*slash == '/' && *(++slash) == '/')

Do you need to check for urls like |http:/|, where this will run of the end of
the string?

r=bbaetz if those invalid urls have been rejected by then.

Comment 8

[Darin Fisher]

Comment on attachment 96687 [details] [diff] [review]
better performing patch

andreas: i think bbaetz is right... we will have to add an additional length
check in there.  either compare slash to EndReading, or check (spec.Length() >
end+1)??

Comment 9

[Andreas Otte]

Attachment: patch with length check

please move forward r= ans sr=

Comment 10

[Andreas Otte]

There are two calls to resource urls during startup which are not stopped by
this new check in ::Init and get only resolved as absolute urls by ::Resolve. I
think that is acceptable.

Comment 11

[Darin Fisher]

Comment on attachment 96911 [details] [diff] [review]
patch with length check

>Index: mozilla/netwerk/base/src/nsStandardURL.cpp

>+    if (baseURI) {
>+        PRUint32 start, end;
>+        // pull out the scheme and where it ends
>+        nsresult rv = ExtractURLScheme(spec, &start, &end, nsnull);
>+        if (NS_SUCCEEDED(rv)) {

i think the spec.Length() check should appear here instead:

	  if (NS_SUCCEEDED(rv) && (spec.Length() > end+1)) {

Comment 12

[Andreas Otte]

Attachment: patch with moved length check

yes, this is better, this will save even more time

Comment 13

[Darin Fisher]

Comment on attachment 96916 [details] [diff] [review]
patch with moved length check

sr=darin

nit:
"if it follows, aSpec is really absolute"

need a comma after follows :)

Comment 14

[Andreas Otte]

Done in my file, but I wont attach another patch for that ... Bradley could you
move your r= to the current patch?

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 96916 [details] [diff] [review]
patch with moved length check

r=bbaetz

Comment 16

[Andreas Otte]

fix is in

Comment 17

[Matthias Versen [:Matti]]

*** Bug 165579 has been marked as a duplicate of this bug. ***

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 96916 [details] [diff] [review]
patch with moved length check

a=[e-mail] for 1.0 branch checkin

Change mozilla1.0.2+ to fixed1.0.2 when checked in.

Comment 19

[Boris Zbarsky [:bzbarsky]]

This caused regressions bug 166085 and bug 166175

Comment 20

[Gayatri Rimola]

Please verify the bug. Once verified, change the keyword fixed1.0.2 to
verified1.0.2 

Comment 21

[benc]

Hi, can someone give me a test or some steps?

Comment 22

[Tom Everingham]

Checked using lxr and the patch is checked in on both the trunk and branch. 

verified.