1385883 - Cannot delete history with IDN

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Alice0775 White]

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.

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 2

[Alice0775 White]

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

Comment 3

[Marco Bonardo [:mak]]

Valentin, any idea what's up here? Did we change representation in a way that stored urls now differ from newly encoded ones?

Comment 4

[Mark Banner (:standard8)]

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

[Mark Banner (:standard8)]

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.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

(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.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

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

[Masatoshi Kimura [:emk]]

(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.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

(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

[Masatoshi Kimura [:emk]]

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.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

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

[Masatoshi Kimura [:emk]]

(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.

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Great catch! Not sure how I managed to miss that one. Thanks a lot!

Comment 14

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba5c87e473504075c92362c8a63e824bfe816572

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Attachment: nsStandardURL - make sure we always check if the host is ascii

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70d8a1284779
nsStandardURL - make sure we always check if the host is ascii r=jduell

Comment 17

[Valentin Gosu [:valentin] (he/him)]

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

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/70d8a1284779

Comment 19

[Alice0775 White]

I have filed a new bug Bug 1386195 - Cannot delete history with "www." + IDN