1232287 - When listing a local folder without putting a slash, links are wrong

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Augustin Trancart [:autra]]

[steps]
- Enter the path of a local folder in the address bar, but without entering the '/' at the end of the path, eg 'file:///home/augustin'
- observe the link of each  files in the folder.

[Expected]
each link is correct: 'Documents' link points to 'file:///home/augustin/Documents'

[Actual]
Links miss the parent folder. IE 'Documents' link points to 'file:///home/Documents' (note the missing 'augustin/')

This does not happen if we entered 'file:///home/augustin/' with a trailing slash.

Reproducible on today's nightly.

Comment 1

[Loic]

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d9251b9fb03efe56b7215ec6e2af2b696c9a8723&tochange=19e419ab3792fa5aad9f531364e72827ea01d34c

Mike Hommey — Bug 1224046 - Remove <base href> from directory listings. r=mcmanus

Comment 2

[Mike Hommey [:glandium]]

WTH? When I tested before landing bug 1224046, opening a local folder would always redirect to the same url with a '/' at the end. It looks like it's not always true. Could we fix that?

Comment 3

[Valentin Gosu [:valentin] (he/him)]

(In reply to Mike Hommey [:glandium] from comment #2)
> WTH? When I tested before landing bug 1224046, opening a local folder would
> always redirect to the same url with a '/' at the end. It looks like it's
> not always true. Could we fix that?

It seems that the FTP appending a / to a dir is a special case here - see nsFtpState::SetContentType
I'm not sure what it would take to achieve the same for other protocols, and what that would break.
I suggest backing out bug 1224046.
Maybe Bobby or Gijs can suggest a better way to work around bug 1184387. (maybe skip the base tag only for resource:// URIs, instead of all protocols?)

Comment 4

[Mike Hommey [:glandium]]

(In reply to Valentin Gosu [:valentin] from comment #3)
> (maybe skip the base tag only for resource:// URIs, instead of all
> protocols?)

IIRC, nsIndexedToHTML::DoOnStartRequest doesn't know that it's handling a resource:// URI, so it's not possible to add a base tag there based on that, at least not directly.

Comment 5

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #4)
> (In reply to Valentin Gosu [:valentin] from comment #3)
> > (maybe skip the base tag only for resource:// URIs, instead of all
> > protocols?)
> 
> IIRC, nsIndexedToHTML::DoOnStartRequest doesn't know that it's handling a
> resource:// URI, so it's not possible to add a base tag there based on that,
> at least not directly.

You can ask nsIChannel for its originalURI, which has the correct scheme.

Comment 6

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1232287 - revert bug 1224046 and fix resource: URIs to not have a base tag, r?glandium,valentin

Bug 1232287 - revert bug 1224046 and fix resource: URIs to not have a base tag, r?glandium,valentin

Comment 7

[:Gijs (he/him)]

Comment on attachment 8698405 [details]
MozReview Request: Bug 1232287 - revert bug 1224046 and fix resource: URIs to not have a base tag, r?glandium,valentin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27977/diff/1-2/

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8698405 [details]
MozReview Request: Bug 1232287 - revert bug 1224046 and fix resource: URIs to not have a base tag, r?glandium,valentin

https://reviewboard.mozilla.org/r/27977/#review25063

Looks good to me.

Comment 9

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/27977/#review25151

It would have been easier to review as two patches: one for the backout and one for the alternative fix.

You'll want this landed on aurora.

Random idea, though: why not only add the base tag is the url doesn't terminate with a /?

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8698405 [details]
MozReview Request: Bug 1232287 - revert bug 1224046 and fix resource: URIs to not have a base tag, r?glandium,valentin

Looks like mozreview didn't propagate the r+ here.

Comment 11

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #9)
> https://reviewboard.mozilla.org/r/27977/#review25151
> 
> It would have been easier to review as two patches: one for the backout and
> one for the alternative fix.

Yeah, I realized this after the fact, sorry. :-\

> You'll want this landed on aurora.

Right y'are.

> Random idea, though: why not only add the base tag is the url doesn't
> terminate with a /?

That seems more likely to cause breakage for reasons hitherto unsuspected. This is essentially a kind of minimal re-fix of bug 1224046. Let's investigate that solution as a followup.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e20ca388caa9

Comment 13

[:Gijs (he/him)]

Comment on attachment 8698405 [details]
MozReview Request: Bug 1232287 - revert bug 1224046 and fix resource: URIs to not have a base tag, r?glandium,valentin

Approval Request Comment
[Feature/regressing bug #]: bug 1224046
[User impact if declined]: links on local file:/// listing pages are wrong
[Describe test coverage new/current, TreeHerder]: no. :-(
[Risks and why]: pretty low risk; essentially a backout of bug 1224046 with a fix added so resource:/// URIs keep working.
[String/UUID change made/needed]: no

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e20ca388caa9

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8698405 [details]
MozReview Request: Bug 1232287 - revert bug 1224046 and fix resource: URIs to not have a base tag, r?glandium,valentin

Fix a regression, taking it.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/88d3b4cf0b29

Comment 17

[Augustin Trancart [:autra]]

Verified fixed on:

Name: Firefox
Version: 46.0a1
Build ID: 20151229030216
Update Channel: nightly
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0
Multiprocess Windows: 2/2 (default: true)
Safe Mode: false

Thanks everyone \o