+yyushkina FYI, the translate UI permits blacklisting (never translate this site) for pages you're visiting while incognito. This leaves a trace of the user which is probably not desired. Correct?

+ Jon for actual behavior. The desired behavior would be for Never translate this site that is chosen in an incognito session to apply only within the context of that session.

Anthony to investigate actual behavior

A fix with significantly lower investment than ideal behavior in #2 is probably just to disable the "Never translate this site" menu option when in incognito.

There would also be less user confusion as a user might not read "never" as "don't do this until end of session". If the fix is expiry at end of session the string would ideally be renamed under incognito to reflect this.

I think I'd prefer the latter - renaming rather than dropping the option altogether - but can live with either.

Hi anthonyvd@, yyushkina@,

Any progress on this bug? This is an important issue from privacy point of view. 

Making this a P2 but note that this behavior doesn't actually leak any info to the site. Anthony and I chatted about this today and he'll tackle this next. We're likely just going to disable the broken options.

Thank you Yana,

The concern from incognito point of view is mostly local privacy. If a user uses a website in incognito, s/he expects that after closing the session, it won't be revealed to other users of the device through any sort of trace.

FWIW, we never show a list of sites in any UI so it would be hard to leak this data to other users.

Yes, I was thinking about putting a feature request for it. :D

But even now that it does not exist, a concerned forensic attacker can check if a website uses the default settings, or is already modified. Hence incognito should not affect the regular mode.

I'd expect the number of users "always translating" in incognito mode to be fairly low, so it still feels like a corner case that a concerned forensic attacker is unlikely to consider.

We should definitely close it but I think the minimum effort method of just disabling the options are definitely sufficient here. Let me know if you want me to pitch in a cycle or two here.

There might be a valid wider concern of the fact that we are able to store this information while in incognito mode but that's a wider concern than this specific instance.

Thanks, my main concern is about storing information in incognito. Is there a separate bug for it?

Not one that I'm aware of. I'm not even sure it's blanket desired.

Note that you can save bookmarks in incognito and that is a significantly more prominent feature (always visible in omnibox). It's pretty clear to me that you're storing something here though. Same if you download something unique to the site in incognito (then the saved file is a trace).

The concern I had with the Translate UI specifically is that it's not *really* clear to the user that something is being stored.

I totally agree with the *really clear* part.

For bookmarks and downloads, user is choosing an action which has clear lasting consequence. But permission are not stored and to give user a consistent experience, I think the translation decisions should not be stored as well.

Maybe we also need to expand the list of expectations on the incognito new tab page.

NTP says "However, downloads and bookmarks will be saved." and this bug is still a bug, right? So the text is working as intended?

Yes, this is still a bug.

I was just saying that if there is a lack of clarity for permissions, it can also be added to the text, as something that is not saved.

+nicolaso to take a look at this. 

I believe the the "Never Translate" modifies the Language List w.r.t the *BlockedLanguage* family of functions in TranslatePref (https://cs.chromium.org/chromium/src/components/translate/core/browser/translate_prefs.h?rcl=1d1b1b67552e3a2d8db118b891d2f129a55d6976&l=175). If it's possible to make TranslatePrefs not persistent in incognito, that'd be the optimal fix. Otherwise, we can fallback to disabling problematic options in the UI in Incognito mode. 

To be clear: TranslatePrefs should apply in Incognito, they should just not be updated.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1bfbebf5eda827adc039016bccfdc938580c8326

commit 1bfbebf5eda827adc039016bccfdc938580c8326
Author: Nicolas Ouellet-payeur <nicolaso@chromium.org>
Date: Thu Jun 21 19:08:08 2018

Don't persist "Never translate this site" blacklist in incognito.

For privacy reasons, we don't want the blacklist to be persisted after the user
leaves incognito mode (as this info could reveal a website that was visited in
incognito).

With this patch, adding a website to the blacklist makes it work for the
incognito browsing session, but only saves those websites in memory. Closing the
incognito discards the changes to the blacklist, and the main browsing session
is not affected.

Bug:  793925 
Change-Id: Icdd35e86b8b807c6a639c5c6ba86786f912e1bd0
Reviewed-on: https://chromium-review.googlesource.com/1110259
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569349}
[modify] https://crrev.com/1bfbebf5eda827adc039016bccfdc938580c8326/chrome/browser/prefs/pref_service_syncable_util.cc

Thank you Nicolas.

There is still one more trace in the following scenario:
1- Go to a website in incognito.
2- Choose never translate this language.
3- You will see the decision in regular mode.

One may argue that 'never translate this language' is not limited to incognito, but as we don't change any other regular mode settings based on changes in incognito, this one should also comply with the general pattern.

Hm, that's a good point, rhalavati@. For consistency, both of the 'Never translate' options should behave the same.

Hell, if we _really_ want to be consistent, we also shouldn't save 'Always translate this site' in incognito. Saving _some_ of the user's choices, but not others, is a bit odd.

Luckily, this should be an easy change to make. I'll take a look at it.

Thanks,
Yes, no incognito decision should be saved.

Hmm, it looks like 'Always translate English' isn't respected in Incognito mode anyways [1]. If it's neither respecting nor saving that setting, then I think we should hide the 'Always translate' option in incognito mode.

[1] https://cs.chromium.org/chromium/src/components/translate/core/browser/translate_manager.cc?l=238&rcl=49d5c30b70308ebf22fe9c14a933840c35d3f189

+1.

By the same logic we should hide "Never" options in Incognito mode too. It's weird to show Never and not Always.

yyushkina@: the main difference is that 'Never' is actually respected in incognito, whereas 'Always' is ignored in incognito. This is to give the user more control over the translation in incognito--again, for privacy reasons.

With my latest patch, 'Never translate this website' still _works_ in incognito, it just doesn't get saved.

But 'Always' never worked in incognito in the first place, it only gets saved; which is weird behavior to begin with. If we change it so it doesn't get saved in incognito, then the 'Always' button becomes completely useless in Incognito.

Another inconsistency: in incognito, we currently hide the 'Always translate' _checkbox_, but we don't hide the option from the combobox. See the attached side-by-side screenshots.

Deleted: inconsistency.png 27.3 KB

	inconsistency.png 27.3 KB View Download

I don't see why "Always" would be ignored in Incognito. Does that make sense to you at all? I think surfacing "always/never translate language" seems OK in Incognito but that that should outlive the incognito session.

I think "never translate this site" isn't reasonable to surface, even if the preference doesn't outlive the session the user won't know that and it sounds a lot like you're saving a trace of the user.

To clarify I think always/never is OK for language (I think it's too broad to expose anything sensitive) but not for site (neither always nor never).

pbos@: 'Always translate' is ignored by incognito, in the sense that incognito won't automatically translate anything when you visit a page in that language.

This is because the browser makes an API call it does a translation. Some users may be uncomfortable with their browser sending information to Google everytime they navigate to a page in an auto-translated language (in incognito mode!).

So we try to give the user more control over the translation, in incognito.

Thanks for the clarification Nicolas. That makes a ton of sense. 
So to summarize:
1) We should not show "Always translate" in the combobox in Incognito mode since it doesn't do anything.
2) Re: "I think "never translate this site" isn't reasonable to surface, even if the preference doesn't outlive the session the user won't know that and it sounds a lot like you're saving a trace of the user." Not sure I agree. We do want to give users as much control of their incognito experience as possible and if they chose to opt into Never translate this site, given Nicolas's patch which results in the pref only being applied for the duration of the session, I think it's fine.

Gotcha, 1) makes sense to me then.

I'm wondering if another string for 2) would be reasonable to let users know it only applies only until end of incognito (so they know they can trust Incognito not to leave a trace). My concern is mostly w/ optics of trusting Incognito, not that exposing the feature in Translate here isn't a valid use case.

After reading that again, I think it's fine as well, diving into a better string for "never" that just applies to the end of the session is probably just going to end up confusing.

I think as we won't have "always translate this language" for incognito, neigher of the "never translate" commands are also not required, because translation will not happen automatically and is only done by direct user action.

Hence, I think, all always/never translate commands can be removed for incognito mode.

Never translates also means "stop showing me this dialog for every single Swedish page", which is reasonable under Incognito as well.

Isn't automatic translation suggestions also disabled in incognito?
I don't get the popups.

No you should see the UI pop up in incognito if you have "Offer to translate" enabled when not in incognito.

OK. It seems that there's something else wrong with my browser as I don't even get them very often in regular mode. But let's not mix that with this thread.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b66775d80a5b36b723fcdbf2af1ecf293336b85d

commit b66775d80a5b36b723fcdbf2af1ecf293336b85d
Author: Nicolas Ouellet-payeur <nicolaso@chromium.org>
Date: Thu Jun 28 17:32:20 2018

Disable "Always translate" and don't save "never translate" in incognito

We already don't save "Never translate <website>". We want the same behavior for
"Never translate <language>" in order to be consistent. Additionally, the
"Always translate" button doesn't work in incognito mode.

1. Remove the "Always translate <language>" button in incognito mode, on desktop
   and Android.

2. Don't persist "Never translate <language>" if clicked in incognito mode, but
   still apply it to the current session.

Needs more work to update the iOS UI.

Bug:  793925 
Change-Id: I53f9bbd8dbcf605ef0682bf993b2835ab0ba92aa
Reviewed-on: https://chromium-review.googlesource.com/1115204
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Hajime Hoshi <hajimehoshi@chromium.org>
Commit-Queue: Nicolas Ouellet-Payeur <nicolaso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571183}
[modify] https://crrev.com/b66775d80a5b36b723fcdbf2af1ecf293336b85d/chrome/android/java/src/org/chromium/chrome/browser/infobar/TranslateCompactInfoBar.java
[modify] https://crrev.com/b66775d80a5b36b723fcdbf2af1ecf293336b85d/chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenu.java
[modify] https://crrev.com/b66775d80a5b36b723fcdbf2af1ecf293336b85d/chrome/android/java/src/org/chromium/chrome/browser/infobar/translate/TranslateMenuHelper.java
[modify] https://crrev.com/b66775d80a5b36b723fcdbf2af1ecf293336b85d/chrome/browser/prefs/pref_service_syncable_util.cc
[modify] https://crrev.com/b66775d80a5b36b723fcdbf2af1ecf293336b85d/chrome/browser/ui/android/infobars/translate_compact_infobar.cc
[modify] https://crrev.com/b66775d80a5b36b723fcdbf2af1ecf293336b85d/chrome/browser/ui/android/infobars/translate_compact_infobar.h
[modify] https://crrev.com/b66775d80a5b36b723fcdbf2af1ecf293336b85d/chrome/browser/ui/views/translate/translate_bubble_view.cc

Tried checking the issue on latest canary 69.0.3476.0 using Windows 10 with the below mentioned steps.
1. Launched chrome
2. In Chrome://settings enabled "Offer to translate pages that aren't in a language you read"
3. Navigated to a non english site, where it prompted to translate the page.
We have observed similar behaviour on clicking option button of the pop-up in latest canary and on the versions without fix. Attaching the screen shot of the same.

@Nicolas Ouellet-payeur: Could you please let us know if we have missed anything in verifying the fix.

Thanks!

Deleted: 793925 CL Verification.jpg 130 KB

	793925 CL Verification.jpg 130 KB View Download

I couldn't reproduce this in incognito mode on 69.0.3476.0 canary on Windows.

Is this incognito? With the patch, "Always translate <language>" should only be hidden in incognito.

I did the following test:

1. Went to language settings and 'Hebrew' was not in the list of languages.
1. Opened https://www.haaretz.co.il/ in incognito.
2. When offered, chose "Never translate Hebrew."
4. Closed incognito.
5. Went to language settings.

Now Hebrew was in the list of languages for which translation is offered.

I believe it shouldn't be there.

rhalavati@: Not close the window and re-open a new Incognito session: you'll notice Hebrew isn't there. We must save Hebrew as a "Never translate" pref for the duration of the incognito session so this is working as intended.

On a separate note: c40 needs to work on iOS. Marking as available for someone from the iOS team to take a look at. 

yyushkina@: regarding #44, I agree that what you mentioned is desired behavior.

My #43 was about residual effect of this when incognito windows are closed. The languages list will continue to show Hebrew then.

Looks like you're right. I can still see the language added from "Never translate" when I close Incognito on the most recent Canary.

Anthony - can you take a look? Doens't seem like Nicolas's fix is wokring.

I tried the following steps today, on Chrome 70.0.3517.0 on Windows, not signed in to any profile:

1. Went to naver.com, in non-incognito mode, got a translation offer bubble from Korean, closed the tab.
2. Went to naver.com, this time incognito, got a translation bubble, selected "Never translate Korean".
3. Went to naver.com, in a new incognito tab, got a translation bubble, selected "Never translate this site".
4. Went to naver.com, in another new incognito tab, did not get a translation bubble.
5. Went to naver.com, in a non-incognito new tab, got a translation bubble. (Even without closing the incognito tabs.)
6. Went to chrome://settings/languages, and Korean was not listed under languages.

It seems to me that Step 3 should not have had a translation bubble appearing (since Step 2 disabled translation from Korean), but other than that everything else seems to be working.

If the problem seen in #47 was not fixed by nicolaso@'s CL at crrev.com/1115204, it may have been fixed as a side-effect of rhalavati@'s changes under  crbug.com/861722 , which changed the preference-saving code from a blacklist (save to the underlying non-incognito profile by default, with listed exceptions) to a whitelist (save in memory by default, with listed exceptions going to the underlying non-incognito profile).

I'll look into why my Step 3 seems to be doing the wrong thing (not even saving the "Never translate Korean" preference for the duration of the incognito session).

After some experimentation with this, here are some observations (still for Chrome 70.0.3517).

The relevant logic for (not) showing a translation box when a language was selected as "Never translate <language>" is in |TranslateManager::InitiateTranslation| (see [1]).

[1] https://cs.chromium.org/chromium/src/components/translate/core/browser/translate_manager.cc?l=219-228

In my testing, the |accept_languages| used there gets updated outside of incognito mode when the user chooses "Never translate <language>" to contain the excluded language, but not in incognito mode. It looks like |accept_languages| is based on the |prefs::kAcceptLanguages| preference.

The relevant logic for changing the user preferences when selecting "Never translate <language>" is in |TranslateUIDelegate::SetLanguageBlocked| (see [2]), which mainly calls |TranslatePrefs::AddToLanguageList| (see [3]) and from there |TranslatePrefs::BlacklistValue| (see [4]). This code also looks like it's based on the preferences |prefs::kAcceptLanguages| (same as above) and |TranslatePrefs::kPrefTranslateBlockedLanguages|.

[2] https://cs.chromium.org/chromium/src/components/translate/core/browser/translate_ui_delegate.cc?l=272-289
[3] https://cs.chromium.org/chromium/src/components/translate/core/browser/translate_prefs.cc?l=234-259
[4] https://cs.chromium.org/chromium/src/components/translate/core/browser/translate_prefs.cc?l=897-910

In my testing, the funny thing is that even in incognito mode, |TranslatePrefs::AddToLanguageList| succeeds in adding the blocked language to the preferences (and to be clear, the preference changes are only visible inside the incognito mode tabs, and only until the end of the incognito session). That is, when selecting "Never translate <language>" for the second time in incognito mode: 

- the function |TranslatePrefs::BlacklistValue| (see [4]) sees the language as already blacklisted; and

- the function |TranslatePrefs::AddToLanguageList| (see [3]) sees the language as already in the |prefs:kAcceptLanguages| preference, since |base::ContainsValue(languages, chrome_language)| is true.


Some theories I'm investigating now:

- Maybe the code in [1] and the code in [2] are somehow using different instances of |PrefService|, possibly because incognito mode is using an overlay?

- Maybe |TranslateAcceptLanguages| is not receiving updates to the |prefs::kAcceptLanguages| preference in incognito mode even though it registers a listener for this preference (see [5])?

[5] https://cs.chromium.org/chromium/src/components/translate/core/browser/translate_accept_languages.cc?l=26-32

After a bit more quick testing:

The update function |TranslateAcceptLanguages::InitAcceptLanguages| seems to only ever be called with the non-incognito |PrefService| instance, so it naturally doesn't get notified when an incognito |PrefService| gets a change to its |prefs::kAcceptLanguages| preference.

I put up a proposed fix for the immediate issue at crrev.com/118654. The underlying issue seems to be that, in Incognito mode, the |PrefService| used by |TranslateClient| and the one used by |TranslatePrefs| in |TranslateManager::InitiateTranslation| are not the same, so they have a different notion of the |prefs::kAcceptLanguages| preference, which affects whether or not the option to translate the page is offered.

With this change, the following steps work:

1. Incognito, go to naver.com, get offered translation, choose "Never translate Korean".
2. Same incognito window, go to naver.com, no offer of translation.
3. Non-incognito window, go to naver.com, get offered translation.
4. Close incognito window and open a new incognito window, go to naver.com, get offered translation.

There is still the question of whether it's OK for the |TranslateClient| used by the Incognito mode tab to refer to the |PrefService| for the underlying non-Incognito profile, and whether other bugs result from this.

crrev.com/1186547

Thank you, I did not go deeply through your CL, but the way you phrase it here, it is NOT OK for the translate client to use the non-incognito pref service when it is called from incognito mode. It will again result in leaking changes to user profile.

Sorry for the typo, the proposed fix is at crrev.com/1186547.

Re #54, I agree that we should be limiting the use of the non-incognito pref service as much as possible (and ideally not use it at all). I'll be moving on to a different team starting Monday, so I probably won't be the one doing it, but here are a few notes for the next person who investigates this issue:

The translate client (or at least the ChromeTranslateClient subclass) explicitly bypasses the incognito pref service in two places that I could find, by calling GetOriginalProfile():

[1] https://cs.chromium.org/chromium/src/chrome/browser/translate/chrome_translate_client.cc?l=274
[2] https://cs.chromium.org/chromium/src/chrome/browser/translate/chrome_translate_client.cc?l=195-196

It's not clear to me why this is the case. Just removing those calls to GetOriginalProfile() did not fix everything in the tests I did.

Separately, the TranslateAcceptLanguages instance available directly from the ChromeTranslateClient (and which was used before crrev.com/1186547) is a cached instance, keyed by the browser context, rather than by a pref service:

[3] https://cs.chromium.org/chromium/src/chrome/browser/translate/chrome_translate_client.cc?l=164

I don't know the relationship between browser contexts and pref services (one-to-one? one-to-many? fixed or changing over time?) but this may be part of the problem.

The browser context is the same as the Profile, and it only has one pref service. This is a one to one relationship.
The incognito profile has its own pref service, that is not shared with the original profile.

See [1] for an update on why my proposed fix is not a good idea.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1186547#message-b6494416ebb56774177df474ef126dc5c8401510

In short, my CL makes TranslatePrefs responsible for creating its own instance of TranslateAcceptLanguages, but in fact there should be a single instance of TranslateAcceptLanguages per PrefService (which, as droger@ points out in #57, is related to a single Profile).

The correct solution is probably to fix TranslateAcceptLanguages so that it has an instance for the incognito Profile which uses the incognito PrefService, but before doing that it would be important to understand why TranslateAcceptLanguages was set up to hook up to the underlying, non-incognito PrefService in the first place.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89bc592546196a93470bfe4aea70e2ab45141140

commit 89bc592546196a93470bfe4aea70e2ab45141140
Author: Anthony Vallee-Dubois <anthonyvd@chromium.org>
Date: Tue Sep 04 17:24:10 2018

Translate fixes in Incognito

This CL refactors the TranslateAcceptLanguages class to be a
KeyedService itself, removing an unnecessary abstraction level. It also
lets OffTheRecord profiles instantiate their own
TranslateAcceptLanguages.

Since OTR profiles get non-persistent, overlayed PrefServices, this
gives access to all translate prefs from the original profile from the
OTR profile without the risk of the wrong data being persisted. It also
makes translate's "Never translate this language" feature useable within
an Incognito session without persisting data in the original profile.

There is also a small cleanup of some calls to GetOriginalProfile that
were unnecessary thanks to the overlayed pref services.

Bug:  793925 
Change-Id: I1396c72945e2c48483fb1a5b4caa636415138d5d
Reviewed-on: https://chromium-review.googlesource.com/1195834
Reviewed-by: Michael Martis <martis@chromium.org>
Commit-Queue: anthonyvd <anthonyvd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588565}
[modify] https://crrev.com/89bc592546196a93470bfe4aea70e2ab45141140/chrome/browser/translate/chrome_translate_client.cc
[modify] https://crrev.com/89bc592546196a93470bfe4aea70e2ab45141140/chrome/browser/translate/translate_accept_languages_factory.cc
[modify] https://crrev.com/89bc592546196a93470bfe4aea70e2ab45141140/components/translate/core/browser/translate_accept_languages.h

Thank you. #43 seems to be fixed.

Anthony - is it ok to mark this as fixed?

This should be fixed as of r588565.