HostContentSettingsMapTest in unit_tests is failing on test-o-phone |
||||
Issue descriptionLink to logs: https://logs.chromium.org/v/?s=chrome%2Fbb%2Finternal.client.clank%2Ftest-o-phone%2F1189%2F%2B%2Frecipes%2Fsteps%2Funit_tests%2F0%2Fstdout C 647.074s Main [CRASH] HostContentSettingsMapTest.IncognitoPartialInheritPref: C 647.074s Main [ RUN ] HostContentSettingsMapTest.IncognitoPartialInheritPref C 647.074s Main [FATAL:notification_channels_provider_android.cc(393)] Check failed: old_channel_status == new_channel_status (1 vs. 0)
,
Nov 21 2017
Yes, these unit tests are hitting the DCHECK on O because the tests call Set/UpdateWebsiteSetting to toggle between ALLOW/BLOCK for the notification content setting, a sequence of events that should never happen on Android O, because on O the user toggles permission by toggling channel status in System UI, which does not go through this code path. For HostContentSettingsMapTest.IncognitoPartialInheritDefault[1], a quick fix would be to simply use a content settings type other than notifications. For NotificationPermissionContextTest.CrossOriginPermissionChecks[2], it's trickier. Maybe we could insert a 'NotificaitonPermissionContext.ResetPermission' in between toggling the notification permission, but only on O, to reflect the change in expected calls on O. Wdyt, Peter, Raymes? [1] https://cs.chromium.org/chromium/src/chrome/browser/content_settings/host_content_settings_map_unittest.cc?q=HostContentSettingsMapTest&sq=package:chromium&l=947 [2] https://cs.chromium.org/chromium/src/chrome/browser/notifications/notification_permission_context_unittest.cc?sq=package:chromium&l=140
,
Nov 21 2017
The plan in #2 sounds roughly good. It's a bit worrying that we don't have trybots to run tests in this configuration? I wonder if that's something we can organize?
,
Nov 22 2017
@raymes, there is an o-phone bot set up now at https://uberchromegw.corp.google.com/i/internal.client.clank/builders/test-o-phone but I don't think it has ever passed yet. Fixing this is a step towards making that bot useful (expect there's many other tests failing too though so it may be some time before that happens).
,
Dec 6 2017
From looking at the unit_tests stdout of the most recent test run on O, I note there is also a third test failing this DCHECK: SiteEngagementServiceTest.NotificationPermission I will proceed with the plan in #2 to fix up these 3 tests later today.
,
Feb 2 2018
It looks like a number of tests in ClearBrowsingDataPreferenceTest are also failing on the O-bot due to this issue: [FATAL:notification_channels_provider_android.cc(392)] Check failed: old_channel_status == new_channel_status (0 vs. 1) Example results details: https://00e9e64baca158ed1692027e9c545d3545523fe3c2de3a7f4d-apidata.googleusercontent.com/download/storage/v1/b/chrome-result-details/o/html%2Fchrome_public_test_apk_test-o-phone_1519_2018_02_02_T13_13_53-UTC?qk=AD5uMEv0oifS8OnYIRmWTgh3ioSlOtsa_oJLu88POlfZ5Eo2bwD8ltHc2udHJDiofAhGl94NH29nh7QDC9ycjtlmsGn1MVB_bBM5FFrPTKZbqtYPaapii9op6GzDcxwJG7KxRHatYo8lVVjxpKWXFwy2h_MCulJpD50GkqGJsB6mFlzKI1hw6Li14W2A77XPKSMQDvVmqsGpk74Fy9HWwYs60L4l-_SEB3yca2KHN3A9bBFjKYoVTJj7XWUQYTl6VwoKURJNsBZndPtf-mEr89jd2tzpAt6AQi6bWyImYUQVBRYGJs14f8-PGMwGZcGm3MB_0FIMFpvp9OOkCDqwEO1nxV_SNq0UG-X-PjA23ymSfIcsip5JcrHAodFdgjZrbAE88HD8BrqzuoBotoRUWYUGf50A_PAkSEYHvAes6_l6X7seIdVOCHGy3fD2MkshmUT74e97bWOFnGJJt33DTQZMmOLmrYktWHaiEl7UuZBCTzNXgGOB2458bGTVTKVGHQ_IL2fWloI3g2JGpJmXD6MFPJShw5pb9RCYdh7Z8SF1QFwozgAzrUvTtVjFek-ybyAkVL9cIDyNAzsffzVZ3Itfpgbv-nmZsv2rLDnAZoJQav00o2K9vUz2Iw9GjKRdbiLAJF9QgcSQqa7bBfv-tZyK6EvD94bzGlYo8qqoOEncox3L-iGnI3yDgMVDFk3YHEqy0xyxCdURNmmcXrf9ZOhJpCX4b0Yk4Rk9DgwK4f4ooL2ECkqUOtAVZSt05zZY8cw3zr8eXWN2DCl9Er5qUFg5nLrUkzUcoKqeP7ituz392NIVT8-XYnJQAhN-7lvR8TEg1SfOIF-3
,
Feb 5 2018
,
Feb 5 2018
Oops, looks like I never did get round to fixing them up as promised in #5. I'm on it now though.
So to summarize the following tests all need fixing up on O due to DCHECKs in NotificationChannelsProviderAndroid:
1. HostContentSettingsMapTest.IncognitoPartialInheritPref
2. NotificationPermissionContextTest.CrossOriginPermissionChecks
3. SiteEngagementServiceTest.NotificationPermission
4. ClearBrowsingDataPreferenceTest.*
Here's the relevant stack trace for 4 from a local repro:
I 4.313s Main 0096857d NotificationChannelsProviderAndroid::CreateChannelIfRequired(std::__ndk1::basic_string<char, std::__n
dk1::char_traits<char>, std::__ndk1::allocator<char> > const&, NotificationChannelStatus)
/usr/local/google/home/awdf/repos/clankium
/src/chrome/browser/notifications/notification_channels_provider_android.cc:392:5
I 4.313s Main 009684d3 NotificationChannelsProviderAndroid::SetWebsiteSetting(ContentSettingsPattern const&, ContentSettings
Pattern const&, ContentSettingsType, std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> >
const&, base::Value*) /usr/local/google/home/awdf/repos/clankium
/src/chrome/browser/notifications/notification_channels_provider_android.cc:310:7
I 4.313s Main 00c2097d HostContentSettingsMap::SetWebsiteSettingCustomScope(ContentSettingsPattern const&, ContentSettingsPa
ttern const&, ContentSettingsType, std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > c
onst&, std::__ndk1::unique_ptr<base::Value, std::__ndk1::default_delete<base::Value> >) /usr/local/google/home/awdf/repos/clankium
/src/components/content_settings/core/browser/host_content_settings_map.cc:421:31
I 4.314s Main 00c20db1 HostContentSettingsMap::SetContentSettingCustomScope(ContentSettingsPattern const&, ContentSettingsPa
ttern const&, ContentSettingsType, std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > c
onst&, ContentSetting) /usr/local/google/home/awdf/repos/clankium
/src/components/content_settings/core/browser/host_content_settings_map.cc:511:3
I 4.314s Main 00c20e1b HostContentSettingsMap::SetContentSettingDefaultScope(GURL const&, GURL const&, ContentSettingsType,
std::__ndk1::basic_string<char, std::__ndk1::char_traits<char>, std::__ndk1::allocator<char> > const&, ContentSetting)
/usr/local/google/home/awdf/repos/clankium
/src/components/content_settings/core/browser/host_content_settings_map.cc:529:3
I 4.314s Main 009679a9 DesktopNotificationProfileUtil::DenyPermission(Profile*, GURL const&)
/usr/local/google/home/awdf/repos/clankium
/src/chrome/browser/notifications/desktop_notification_profile_util.cc:40:9
I 4.314s Main v------> JNI_WebsitePreferenceBridge_SetNotificationSettingForOrigin(_JNIEnv*, base::android::JavaParamRef<_jc
lass*> const&, base::android::JavaParamRef<_jstring*> const&, int, unsigned char)
/usr/local/google/home/awdf/repos/clankium
/src/chrome/browser/android/preferences/website_preference_bridge.cc:441:7
I 4.314s Main 00a36017 Java_org_chromium_chrome_browser_preferences_website_WebsitePreferenceBridge_nativeSetNotificationSet
tingForOrigin
/usr/local/google/home/awdf/repos/clankium
/src/out/AndroidDebug/gen/chrome/browser/jni_headers/chrome/jni/WebsitePreferenceBridge_jni.h:264:0
,
Feb 5 2018
Okay, so far I have fixes for tests 1 & 2 ready for review, and looks like the test in 3 has since been deleted. Re: 4, ClearBrowsingDataPreferenceTest, debugging revealed that it's crashing during the test setUp: the current strategy to reset permission for the DSE is in fact triggering code that re-grants the permission somehow, at this line: https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java?l=82 notificationSettings.setContentSetting(ContentSetting.DEFAULT) It hits NCPA::SetWebsiteSetting twice - once with value DEFAULT, then secondly with ALLOW, bizarrely. @Raymes can you take that particular test on perhaps? (Feel free to split out a new bug).
,
Feb 8 2018
https://chromium-review.googlesource.com/c/chromium/src/+/902204 should go in later today to fix 1 and 2
,
Feb 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fec34c07e5d525a06cc10f6494231dd0ec18c6d3 commit fec34c07e5d525a06cc10f6494231dd0ec18c6d3 Author: Anita Woodruff <awdf@chromium.org> Date: Thu Feb 08 18:03:20 2018 [Tests] Don't set notification content settings on Android O - Previously tests that called SetWebsiteSetting to toggle the notification content setting between ALLOW/BLOCK failed a DCHECK on Android O, because this behaviour is not expected or allowed on O. - Avoid this by using other content settings or explicitly resetting the content setting to DEFAULT on Android O. R=raymes@chromium.org Bug: 787218 Change-Id: I6cc5839178f5883862ab648b601a722d0bff9524 Reviewed-on: https://chromium-review.googlesource.com/902204 Commit-Queue: Anita Woodruff <awdf@chromium.org> Reviewed-by: Bernhard Bauer <bauerb@chromium.org> Reviewed-by: Peter Beverloo <peter@chromium.org> Cr-Commit-Position: refs/heads/master@{#535436} [modify] https://crrev.com/fec34c07e5d525a06cc10f6494231dd0ec18c6d3/chrome/browser/content_settings/host_content_settings_map_unittest.cc [modify] https://crrev.com/fec34c07e5d525a06cc10f6494231dd0ec18c6d3/chrome/browser/notifications/notification_permission_context_unittest.cc
,
Feb 12 2018
awdf: unfortunately I don't have things set up to test this easily - are you able to help? When I modified this test, I was pretty sure I manually tested in on O. I wonder if there is a race condition here where the default search engine is being initialized after the value is changed. It may help to move both calls to notificationSettings.setContentSetting() such that they happen sequentially on the UI thread rather than bouncing back to the test thread first.
,
Feb 16 2018
@Raymes - re your suggestion in #12, I tried that and it didn't help. I agree it is probably due to the order of initialization that the DEFAULT -> ALLOW pattern occurs in response to the first setContentSetting(DEFAULT) call. But interestingly, inserting another setContentSetting(DEFAULT) didn't resolve the issue. Any further ideas? If you don't have an O+ device to repro on, I suggest you change this method to return true to enable the NotificationChannelsProvider on all versions of Android while testing: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationSettingsBridge.java?q=NotificationSettingsBridge.j&sq=package:chromium&l=25
,
Feb 16 2018
.. although actually you may end hitting many more DCHECKS/crashes doing that, so you might just have to go find an O phone, sorry!
,
Mar 8 2018
Closing this one as Fixed since the HostContentSettingsMapTest failures mentioned in the description are now resolved. The ClearBrowsingDataPreferencesTest failures discussed above have their own bug now at Issue 808600 , let's move the discussion there. |
||||
►
Sign in to add a comment |
||||
Comment 1 by mariakho...@chromium.org
, Nov 21 2017Owner: awdf@chromium.org