The default bug view has changed. See this FAQ.

firefox tries to load incorrect url if urlbar autocomplete suggests first-two-letters-duplicated hosts when input url contains % encoded components

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Location Bar
P1
normal
VERIFIED FIXED
a month ago
a day ago

People

(Reporter: uFFFD, Assigned: mak)

Tracking

(Depends on: 1 bug, {regression})

51 Branch
Firefox 54
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 wontfix, firefox52 wontfix, firefox53? fixed, firefox54 verified, firefox-esr45 unaffected, firefox-esr52 wontfix)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Reporter)

Description

a month ago
Created attachment 8837222 [details]
screencast firefox 51.0

the first-two-letters-duplicated hosts bug has already been described in bug 767364
but it is even worse starting from 51.0


Steps to Reproduce:
0. start a local http server on port 80
1. open firefox and load http://localhost/a=&6
2. focus on urlbar and delete `&6`
3. input `%266`
4. delete `%266`
5. input `&6`
6. move input cursor between `&` and `6`
7. delete `&`
8. input `%2`
9. press enter


Expected Result:
1. after 3rd step
    urlbar inline autocomplete shows "localhost/a=&6"
    first entry in urlbar autocomplete dropdown list is "localhost/a=&6 - Visit"

2. after 8th step
    first entry in urlbar autocomplete dropdown list is "localhost/a=&6 - Visit"

3. after 9th step
    firefox loads http://localhost/a=%26


Actual Result:
1. after 3rd step
    urlbar inline autocomplete shows "lolocalhost/a=&6"
    first entry in urlbar autocomplete dropdown list is "lolocalhost/a=&6 - Visit"

2. after 8th step
    first entry in urlbar autocomplete dropdown list is "lolocalhost/a=&6 - Visit"

3. after 9th step
    firefox tries to load http://lolocalhost/a=&6
(Reporter)

Updated

a month ago
Depends on: 767364
(Reporter)

Comment 1

a month ago
Created attachment 8837226 [details]
screencast firefox 50.0

in firefox 50.0
it still loads http://localhost/a=%26 even if autocomplete suggests `lolocalhost/a=&6 - Visit`
(Reporter)

Comment 2

a month ago
Created attachment 8837231 [details]
dumbserv.py

This is the stupid simple local server I used in comment 0

You can use httpbin for testing
http://httpbin.org/get
https://httpbin.org/get

The first-two-letters-duplicated bug also happens when I use ip address instead of hostname
e.g.: 127.0.0.1 => 12127.0.0.1
(Assignee)

Updated

a month ago
Priority: -- → P2
Whiteboard: [fxsearch]
(Assignee)

Updated

a month ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

a month ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Assignee)

Updated

a month ago
Blocks: 1340601
(Assignee)

Updated

a month ago
Priority: P2 → P1
(Assignee)

Comment 3

a month ago
the bug lies in the strippedPrefix calculation in unifiedComplete.js, since _searchString goes through unescapeURIForUI that transforms it. then we calculate a len diff with the original string, that is wrong.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

a month ago
mozreview-review
Comment on attachment 8839857 [details]
Bug 1339497 - Location bar may suggest an incorrect url when input url contains % encoded components.

https://reviewboard.mozilla.org/r/114452/#review116340

Looks good.
Attachment #8839857 - Flags: review?(standard8) → review+
(Assignee)

Updated

a month ago
Blocks: 1341081

Comment 7

a month ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/ff243ef88531
Location bar may suggest an incorrect url when input url contains % encoded components. r=standard8
[Tracking Requested - why for this release]:
This problem seems to have gotten worse in Fx51--you are actually taken to the wrong site, rather than it merely appearing wrong in the dropdown. As a regression it would be nice to fix in 52 so it's not broken on the ESR-52 branch, but I don't know how common triggering URLs might be.

(I marked 45 "unaffected", but it's affected by an older variant bug 767364)
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → affected
tracking-firefox52: --- → ?
Keywords: regression
Duplicate of this bug: 1340601

Comment 10

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff243ef88531
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(Assignee)

Comment 11

a month ago
Comment on attachment 8839857 [details]
Bug 1339497 - Location bar may suggest an incorrect url when input url contains % encoded components.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: if the user types or pastes a url with encoded components, and that url is in history, we may send him to the wrong destination.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no, automated tests
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: slightly
[Why is the change risky/not risky?]: there is a small risk of regressions, but this code has lots of tests, and likely only url autoFill ("complete url to the next slash") may be at risk.
[String changes made/needed]: none
Attachment #8839857 - Flags: approval-mozilla-beta?
Attachment #8839857 - Flags: approval-mozilla-aurora?
Marco, the patch doesn't apply:

Merge failed for commit ff243ef88531 (parent 3629511a266b)
grafting 382148:ff243ef88531 "Bug 1339497 - Location bar may suggest an incorrect url when input url contains % encoded components. r=standard8" 
merging toolkit/components/places/UnifiedComplete.js 
merging toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js 
merging toolkit/components/places/tests/unifiedcomplete/xpcshell.ini 
warning: conflicts while merging toolkit/components/places/UnifiedComplete.js! 
(edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(mak77)

Updated

a month ago
status-firefox53: --- → affected
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
(Assignee)

Comment 14

a month ago
Created attachment 8841471 [details] [diff] [review]
Uplift patch

This uplift patch also includes the patch for bug 1322747, that is what is causing the bitrot. I think it's less risky to take both, than to deeply modify this patch, and that bug is quite a nice addition for users (we should have asked for uplift before). In the worst case we'll just take it in Aurora :/
Running on Try (modulo possible issues with pushing Aurora to Try)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62e4372f7702fb27ccfcfa3a5ab1dff1873ffa9f
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #11)

> Bug 1339497 - Location bar may suggest an incorrect url when input url
> contains % encoded components.
> [Needs manual test from QE? If yes, steps to reproduce]: no, automated tests
> [List of other uplifts needed for the feature/fix]: none
> [Is the change risky?]: slightly

Hi Gerry/Marco,

Could you please clarify this out, is there manual testing QA need fr this bug? From Marco's comment 11 understood that manual QA is not needed, but then I get the manual testing request from Gerry.

Anyway, I try testing this bug, and I could not reproduce this, neither on latest Nightly 54.0a1, Build ID 20170227030203(with the fix), nor on a build before the fix to be landed, Nightly 54.0a1 with build ID 20170210030206. I have tested with the steps from bug description, and firefox loads http://localhost/a=%26 on both builds.
Flags: needinfo?(mak77)
Flags: needinfo?(gchang)
Flags: needinfo?(brindusa.tot)
(Assignee)

Comment 16

a month ago
(In reply to Brindusa Tot[:brindusat] from comment #15)
> Could you please clarify this out, is there manual testing QA need fr this
> bug? From Marco's comment 11 understood that manual QA is not needed, but
> then I get the manual testing request from Gerry.

Since there is an automated test it's not strictly needed, but I suppose Releng may want someone to verify the fix for the uplift.

> Anyway, I try testing this bug, and I could not reproduce this, neither on
> latest Nightly 54.0a1, Build ID 20170227030203(with the fix), nor on a build
> before the fix to be landed

There are simpler STR at https://bugzilla.mozilla.org/show_bug.cgi?id=1340601#c3
Flags: needinfo?(mak77)

Updated

a month ago
Duplicate of this bug: 1343178
Comment on attachment 8839857 [details]
Bug 1339497 - Location bar may suggest an incorrect url when input url contains % encoded components.

I think it's probably too late for 52
Attachment #8839857 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox52: affected → wontfix
tracking-firefox-esr52: --- → ?
[Tracking Requested - why for this release]: moving tracking flag from 52 to 53 as we're about to go to release.

We could still consider an uplift to esr52 if this lands and is verified in 53.
tracking-firefox52: ? → ---
tracking-firefox53: --- → ?
Comment on attachment 8839857 [details]
Bug 1339497 - Location bar may suggest an incorrect url when input url contains % encoded components.

Fix an incorrect suggestion issue from location bar if the input contains encoded components. Aurora53+.
Flags: needinfo?(gchang)
Attachment #8839857 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Marco Bonardo [::mak] from comment #16)
> (In reply to Brindusa Tot[:brindusat] from comment #15)
> > Could you please clarify this out, is there manual testing QA need fr this
> > bug? From Marco's comment 11 understood that manual QA is not needed, but
> > then I get the manual testing request from Gerry.
> 
> Since there is an automated test it's not strictly needed, but I suppose
> Releng may want someone to verify the fix for the uplift.
> 
> > Anyway, I try testing this bug, and I could not reproduce this, neither on
> > latest Nightly 54.0a1, Build ID 20170227030203(with the fix), nor on a build
> > before the fix to be landed
> 
> There are simpler STR at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1340601#c3

Verified with steps specified at https://bugzilla.mozilla.org/show_bug.cgi?id=1340601#c3, and I can confirm that the issue is reproducible on an older build, Nightly 54.0a1, build ID 20170210030206 and it is fixed on latest Nightly 54.0a1, build ID 20170301030202.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified

Comment 22

28 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f81626a3260
status-firefox53: affected → fixed
Flags: in-testsuite+
The uplift patch doesn't apply cleanly to esr52 (conflicts in UnifiedComplete.js). Is it worth a rebase and uplift request or should we wontfix?
Flags: needinfo?(mak77)
(Assignee)

Comment 24

a day ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> The uplift patch doesn't apply cleanly to esr52 (conflicts in
> UnifiedComplete.js). Is it worth a rebase and uplift request or should we
> wontfix?

The risk to cause regressions in the location bar due to a wrong unbitrot is too high, the changes to unifiedComplete.js are quite substantial. I'm sorry but I feel like this is a wontfix on esr52. This bug exists from when unified complete was enabled, thus it's not so recent, and it won't make a huge difference to not have the fix there.
status-firefox-esr52: affected → wontfix
tracking-firefox-esr52: ? → ---
Flags: needinfo?(mak77)
You need to log in before you can comment on or make changes to this bug.