Closed Bug 1232287 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Networking, defect)

45 Branch
x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox44 --- unaffected
firefox45 + fixed
firefox46 --- fixed

People

(Reporter: autra, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(1 file)

[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.
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
Blocks: 1224046
Component: Tabbed Browser → Networking
Flags: needinfo?(mh+mozilla)
Keywords: regression
Product: Firefox → Core
Version: Trunk → 45 Branch
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?
Flags: needinfo?(mh+mozilla) → needinfo?(mcmanus)
Flags: needinfo?(mcmanus) → needinfo?(valentin.gosu)
(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?)
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bobbyholley)
(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.
(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.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(bobbyholley)
Bug 1232287 - revert bug 1224046 and fix resource: URIs to not have a base tag, r?glandium,valentin
Attachment #8698405 - Flags: review?(valentin.gosu)
Attachment #8698405 - Flags: review?(mh+mozilla)
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 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.
Attachment #8698405 - Flags: review?(valentin.gosu) → review+
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 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.
Attachment #8698405 - Flags: review?(mh+mozilla) → review+
(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 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
Attachment #8698405 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/e20ca388caa9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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.
Attachment #8698405 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1233776
You need to log in before you can comment on or make changes to this bug.