*In particular we want to test the code for displaying/changing cookie exceptions on android.

Hi all,

I added UI on android to allow adding custom cookie exceptions(like JavaScript) and the page crashed. Test steps are:

1. go to settings->site settings->Cookies
2. change default cookie setting to be block
3. tap on 'add site' which appears below
4. in the popup dialog, input 'www.example.com' and tap 'add' to add an exception for this url
5. tap on www.example.com which is in the exception list
6. in the single website setting page, tap on Cookie permission and try to toggle settings between allow and block for www.example.com
7. the page stopped working and exit

The main reason of the crash is that when trying to modify settings, it will call SetCookieSettingForOrigin() (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/android/preferences/website_preference_bridge.cc&l=458). It fails to construct valid url from jstring origin since it doesn't have scheme specified.

Another thing I noticed is, on android I can't input url or pattern with schemes (http:// or https://) when adding custom exceptions. It is regarded as invalid input and remains red. But I can do it on desktop.

Also, when changing custom site settings, it should call SetContentSettingForPattern() instead of SetCookieSettingForOrigin() because user input might be patterns.

So I prefer to use ContentSettingException for Cookies, like JavaScript and Popups.

Also I'm worrying about notifications on Android (please correct me if I'm wrong). 

As I tested today, when toggling notification exceptions, it uses NotificationInfo which will SetNotificationSettingForOrigin. It doesn't have any problems for now, since clicking allow/block will generate correct url for SetContentSettingDefaultScope(). But users can manually add pattern exceptions on e.g.Linux. If syncing preferences to Android and use the same UI to toggle exceptions, it might crash.

Other types are ok. Only JavsScript and Popup are domain scoped types, and I think that is the reason why they have switched to use ContentSettingException. Other types don't allow users to manually add exceptions on other platforms.

@finnur: does my concern make sense?

I think you are right to be concerned. I was just looking into a crash today where entering manual exceptions for JavaScript and then trying to Clear and Reset the site would crash (because the exception omitted the scheme and during Clear and Reset we call WebsiteAddress::getOrigin, instead of WebsiteAddress::getTitle [1]. 

I'm not entirely sure if there's a general way we should handle this. Where does it crash for you? 

[1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsiteAddress.java&q=WebsiteAddress&sq=package:chromium&type=cs&l=83

For notifications and the current UI, I haven't met any crash. Just worrying about if we synchronize desktop preference (along with all the pattern exceptions on the desktop) to Android and allow users to toggle their settings, it will crash(most likely) in NotificationInfo::setNativePreferenceValue [1], when trying to treat patterns as urls.

[1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/NotificationInfo.java&l=22

Yeah, I think we'll need to be careful when enabling syncing for this for Android.

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

commit 9c6944d530898d73c02325c0a5c03a257dd7f208
Author: lshang <lshang@chromium.org>
Date: Wed Apr 27 03:22:33 2016

Switch Cookie to use ContentSettingException instead of CookieInfo on Android

We are about to start syncing cookie exceptions to Android. After that allowing
users to add exceptions or toggle existing exceptions will cause crashes. It is
because for now CookieInfo only support set settings for urls.

In this CL, CookieInfo is replaced with ContentSettingException, similar to
JavaScript, to support patterns. This will make sure that users can manually
add an exception and toggle it on Android.

BUG= 598909 

Review URL: https://codereview.chromium.org/1864163005

Cr-Commit-Position: refs/heads/master@{#389979}

[delete] https://crrev.com/6a87dc1a00f0bcea007bbf7658e7f45c2b0d2db5/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/CookieInfo.java
[modify] https://crrev.com/9c6944d530898d73c02325c0a5c03a257dd7f208/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/SingleWebsitePreferences.java
[modify] https://crrev.com/9c6944d530898d73c02325c0a5c03a257dd7f208/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/Website.java
[modify] https://crrev.com/9c6944d530898d73c02325c0a5c03a257dd7f208/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePermissionsFetcher.java
[modify] https://crrev.com/9c6944d530898d73c02325c0a5c03a257dd7f208/chrome/android/java/src/org/chromium/chrome/browser/preferences/website/WebsitePreferenceBridge.java
[modify] https://crrev.com/9c6944d530898d73c02325c0a5c03a257dd7f208/chrome/android/java_sources.gni
[modify] https://crrev.com/9c6944d530898d73c02325c0a5c03a257dd7f208/chrome/browser/android/preferences/website_preference_bridge.cc

Mark this as fixed but feel free to reopen it for any future concerns.