Closed Bug 1385883 Opened 7 years ago Closed 7 years ago

Cannot delete history with IDN

Categories

(Toolkit :: Places, defect, P1)

56 Branch
defect

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)

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.
Version: 50 Branch → 56 Branch
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.
Priority: -- → P1
Whiteboard: [fxsearch]
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
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)
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.
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.
(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]
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.
(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.
(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.
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.
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.
(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.
Great catch! Not sure how I managed to miss that one. Thanks a lot!
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
Attachment #8892156 - Flags: review+
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/70d8a128477916173272ffddfaa34538dfc54010
Bug 1385883 - nsStandardURL - make sure we always check if the host is ascii r=jduell
https://hg.mozilla.org/mozilla-central/rev/70d8a1284779
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1386195
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.