Closed Bug 1339497 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Address Bar, defect, P1)

51 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 54
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 ? fixed
firefox54 --- verified

People

(Reporter: xefbfbd, Assigned: mak)

References

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(5 files)

Attached video 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
Depends on: 767364
Attached video screencast firefox 50.0
in firefox 50.0
it still loads http://localhost/a=%26 even if autocomplete suggests `lolocalhost/a=&6 - Visit`
Attached file 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
Priority: -- → P2
Whiteboard: [fxsearch]
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Blocks: 1340601
Priority: P2 → P1
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 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+
Blocks: 1341081
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)
https://hg.mozilla.org/mozilla-central/rev/ff243ef88531
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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)
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)
Attached patch Uplift patchSplinter Review
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)
(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)
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-
[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.
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
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)
(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.
Flags: needinfo?(mak77)
You need to log in before you can comment on or make changes to this bug.