Closed
Bug 1385883
Opened 6 years ago
Closed 6 years ago
Cannot delete history with IDN
Categories
(Toolkit :: Places, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | ? | fixed |
People
(Reporter: alice0775, Assigned: valentin)
References
()
Details
(Keywords: privacy, regression, Whiteboard: [fxsearch][necko-active])
Attachments
(1 file)
6.93 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
Cannot delete history with IDN since Nightly56.0a1. Reproducible : always Steps To Reproduce: 1. Open URL 2. Attempt to delete the history item (Right click and chose "Delete Page" or Select the item and delete from Organize menu in library) Actual Results: Cannot delete the item Expected Results: Should be able to delete the item Interestingly, Bookmark can be deleted even if IDN url.
![]() |
Reporter | |
Updated•6 years ago
|
Version: 50 Branch → 56 Branch
Comment 1•6 years ago
|
||
Interesting, I wonder if this is somehow related to the moving from sync to async (and from nsIURI to URL). This is something to investigate for privacy.
Keywords: privacy,
regressionwindow-wanted
Priority: -- → P1
Updated•6 years ago
|
Whiteboard: [fxsearch]
![]() |
Reporter | |
Comment 2•6 years ago
|
||
Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5ec303752dae48cf77d2f4e7486c0f80c7cc4e67&tochange=82e7b5e70e2396f5c369110da84d5faf928280ff Regressed by: Bug 945240 - Change nsStandardURL's internal hostname representation to punycode
Blocks: 945240
![]() |
Reporter | |
Updated•6 years ago
|
Keywords: regressionwindow-wanted
Comment 3•6 years ago
|
||
Valentin, any idea what's up here? Did we change representation in a way that stored urls now differ from newly encoded ones?
Flags: needinfo?(valentin.gosu)
Comment 4•6 years ago
|
||
To add some info from the places side: - In this case, the url is stored in the database in punycode format (https://www.xn--80ak6aa92e.com/) - When we delete the page we've got the punycode url from the UI - We then do some url harmonisation to get all the URLs consistent, this is basically via: url = new URL(page) - We then do a query on the DB to delete all the urls. This does `url.href`. I guess that before we used to get the punycode representation, but now we get the unicode representation. There appears to be no way on the URL object to get the punycode representation. Now I've seen that, I notice that `network.standard-url.punycode-host` is still set to false (https://hg.mozilla.org/integration/autoland/rev/2dca6b785721) - that was a temporary change. Setting the pref to true fixes the issue for me. Bug 1380617 looks to be setting the pref back to true, but it is a bit worrying that it hasn't landed yet, and 56 is going to beta in a day or so.
Comment 5•6 years ago
|
||
We should probably track this, since it is a user impact from the privacy perspective, and the pref change is a change from what was being shipped previously.
tracking-firefox56:
--- → ?
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Marco Bonardo [::mak] (Away 4-20 August) from comment #3) > Valentin, any idea what's up here? Did we change representation in a way > that stored urls now differ from newly encoded ones? All of the APIs for nsIURI should still return the same output (until we flip the pref). This bugs either means that there is a bug in enforcing the pref, or maybe a method that I missed. (In reply to Mark Banner (:standard8) from comment #4) > To add some info from the places side: > > - In this case, the url is stored in the database in punycode format > (https://www.xn--80ak6aa92e.com/) > - When we delete the page we've got the punycode url from the UI > - We then do some url harmonisation to get all the URLs consistent, this is > basically via: > > url = new URL(page) > > - We then do a query on the DB to delete all the urls. This does `url.href`. > > I guess that before we used to get the punycode representation, but now we > get the unicode representation. There appears to be no way on the URL object > to get the punycode representation. > If we got a punycode href before, and we get IDNA now, that's a bug. I'm looking into it. > Now I've seen that, I notice that `network.standard-url.punycode-host` is > still set to false > (https://hg.mozilla.org/integration/autoland/rev/2dca6b785721) - that was a > temporary change. > > Setting the pref to true fixes the issue for me. > > Bug 1380617 looks to be setting the pref back to true, but it is a bit > worrying that it hasn't landed yet, and 56 is going to beta in a day or so. We'll be landing bug 1380617 after the merge day, since there are a bunch places in the UI which need to be fixed, and we weren't sure we could get to all of them in time.
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Whiteboard: [fxsearch] → [fxsearch][necko-active]
Assignee | ||
Comment 7•6 years ago
|
||
Also, the behaviour I find is that the page does get removed from the history, but the page isn't immediately removed from the list. If I click on the Downloads section, then back on History, the page is gone from the list.
Comment 8•6 years ago
|
||
(In reply to Valentin Gosu [:valentin] from comment #6) > If we got a punycode href before, and we get IDNA now, that's a bug. I'm > looking into it. What does it mean? I thought IDNA == punycode. I found that the "Location" column of the Library used to be IDN == Unicode == "https://www.аррӏе.com/" in Release, but it is IDNA == punycode == "https://www.xn--80ak6aa92e.com/" in Nightly now.
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #8) > (In reply to Valentin Gosu [:valentin] from comment #6) > > If we got a punycode href before, and we get IDNA now, that's a bug. I'm > > looking into it. > > What does it mean? I thought IDNA == punycode. I meant IDN/unicode. > > I found that the "Location" column of the Library used to be IDN == Unicode > == "https://www.аррӏе.com/" in Release, but it is IDNA == punycode == > "https://www.xn--80ak6aa92e.com/" in Nightly now.
Comment 10•6 years ago
|
||
If I visit "https://www.аррӏе.com/" with the old build and then upgrade Nightly to latest, the Location column changes to "https://www.xn--80ak6aa92e.com/". So I think this is a pure UI issue rather than a backend one.
Assignee | ||
Comment 11•6 years ago
|
||
I still think there's a problem with the URL implementation. My changes should not have caused the nsIURI APIs to return punycode if the previous build didn't. I'm still trying to track down where the issue is coming from.
Comment 12•6 years ago
|
||
(In reply to Valentin Gosu [:valentin] from comment #11) > I still think there's a problem with the URL implementation. You are right. Probably I found the cause. Cu.import("resource://gre/modules/NetUtil.jsm"); var uri=NetUtil.newURI("https://www.xn--80ak6aa92e.com/"); uri.ref=""; uri.spec; Nightly: https://www.xn--80ak6aa92e.com/ Release: https://www.аррӏе.com/ Because SetRef will call InvalidateCache which will in turn reset mCheckedIfHostA and mDisplayHost. So GetDisplaySpec will not work correctly. If mCheckedIfHostA is false, GetDisplaySpec should call NormalizeIDN.
Assignee | ||
Comment 13•6 years ago
|
||
Great catch! Not sure how I managed to miss that one. Thanks a lot!
Assignee | ||
Comment 14•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba5c87e473504075c92362c8a63e824bfe816572
Assignee | ||
Comment 15•6 years ago
|
||
SetRef and several other methods call InvalidateCache, which in turn sets mCheckedIfHostA to false. That means that each time before trying to access mDisplayHost we need to make sure it is properly initalized. MozReview-Commit-ID: Agtsy6Lx7Nb
Updated•6 years ago
|
Attachment #8892156 -
Flags: review+
Comment 16•6 years ago
|
||
Pushed by valentin.gosu@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/70d8a1284779 nsStandardURL - make sure we always check if the host is ascii r=jduell
Assignee | ||
Comment 17•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/70d8a128477916173272ffddfaa34538dfc54010 Bug 1385883 - nsStandardURL - make sure we always check if the host is ascii r=jduell
![]() |
||
Comment 18•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/70d8a1284779
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
![]() |
Reporter | |
Comment 19•6 years ago
|
||
I have filed a new bug Bug 1386195 - Cannot delete history with "www." + IDN
You need to log in
before you can comment on or make changes to this bug.
Description
•