New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 841203 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 3
Type: Bug



Sign in to add a comment

NotificationsTest.TestOriginPrefsNotSavedInIncognito should not fail

Project Member Reported by awdf@chromium.org, May 9 2018

Issue description

This interactive ui test was disabled with a comment linking it to a crash which was fixed in 2012 ( Issue 160657 ), so it's probably time to re-enable it.

https://cs.chromium.org/chromium/src/chrome/browser/notifications/notification_interactive_uitest.cc?l=365 

However I just tried un-disabling & running it on Linux with the following command and it now fails:

ninja -C out/LinuxDebug -j2000 -l20 interactive_ui_tests && out/LinuxDebug/interactive_ui_tests --gtest_filter="*NotificationsTest.TestOriginPrefsNotSaved*" --extract-test-list-from-filter

../../chrome/browser/notifications/notification_interactive_uitest.cc:377: Failure
Value of: RequestAndAcceptPermission(incognito)
  Actual: false
Expected: true




 

Comment 1 by awdf@chromium.org, May 14 2018

Status: Started (was: Assigned)
Just ran it again with the same command as above, and this time it crashed when initializing the test suite :

Received signal 11 SEGV_MAPERR 0000000000e8
#0 0x7f841dfe9aed base::debug::StackTrace::StackTrace()
#1 0x7f841dd134bc base::debug::StackTrace::StackTrace()
#2 0x7f841dfe9544 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7f8420dfb0c0 <unknown>
#4 0x0000039924e8 views::test::(anonymous namespace)::UIControlsDesktopX11::UIControlsDesktopX11()
#5 0x000003992488 views::test::CreateUIControlsDesktopAura()
#6 0x0000019a8fdf InteractiveUITestSuite::Initialize()
#7 0x000004a44570 base::TestSuite::Run()
#8 0x0000019a8ed8 InteractiveUITestSuiteRunner::RunTestSuite()
#9 0x000003a42f03 ChromeTestLauncherDelegate::RunTestSuite()
#10 0x000004896cf9 content::LaunchTests()
#11 0x000003a441f2 LaunchChromeTests()
#12 0x0000019a8d43 main
#13 0x7f83fe9ce2b1 __libc_start_main
#14 0x00000170002a _start
  r8: 00007ffe12ec93d8  r9: 00007ffe12ec93d8 r10: 00007ffe12ec90ff r11: 00007f83feb17e00
 r12: 0000000001700000 r13: 00007ffe12eca600 r14: 0000000000000000 r15: 0000000000000000
  di: 0000000000000000  si: 0000140dcfb6ae00  bp: 00007ffe12ec9780  bx: 0000000000000000
  dx: 0000000000000000  ax: 0000000000000000  cx: 0000000000000000  sp: 00007ffe12ec9720
  ip: 00000000039924e8 efl: 0000000000010246 cgf: 002b000000000033 erf: 0000000000000004
 trp: 000000000000000e msk: 0000000000000000 cr2: 00000000000000e8
[end of stack trace]

Comment 2 by awdf@chromium.org, May 14 2018

Seems the crash in #1 occurs reliably on one of my monitors but not the other. Weird.

Comment 3 by peter@chromium.org, May 14 2018

The crash in #c3 is unrelated to the failure here. Reading the code, the ASSERT on notification_interactive_uitest.cc:378 is verifying that permission was *granted* in an Incognito context. We never grant permission in an Incognito context. Is the test just void?

Comment 4 by awdf@chromium.org, May 14 2018

Agree this test should not be testing that permission can be granted in incognito.

However we should have *something* testing that the notification auto-denial is not persisted after the incognito session.

NotificationPermissionContextTest.TestDenyInIncognitoAfterDelay currently tests that the autoblock occurs but not that this block is not persisted. Maybe we can add another line there to do so? https://cs.chromium.org/chromium/src/chrome/browser/notifications/notification_permission_context_unittest.cc?l=281

Comment 5 by awdf@chromium.org, May 14 2018

Cc: rhalavati@chromium.org
Components: Privacy>Incognito
Peter pointed out that the general permission/content-settings tests should have us covered here, since *all* incognito settings should only be stored in memory for the duration of the incognito session, not just notification settings. So this specialized legacy test can be deleted.

However after a cursory search I could not find any test for incognito settings not being persisted after the incognito session has closed. But maybe I'm missing something?

Comment 6 by awdf@chromium.org, May 16 2018

Cc: msramek@chromium.org dullweber@chromium.org
Labels: Review-Privacy
Can anyone from the Privacy team comment on #5? 

ie. 

1. Is it indeed true to say '*all* incognito settings should only be stored in memory for the duration of the incognito session and no longer'?

2. Do we have adequate test coverage for this?
Yes, *all* incognito settings are temporary and should note be written to disk, but I am not sure about the test coverage.
My plan in this quarter and the next one is to check for it and make sure it is complete.

So we can assign this one to me as well, and based on the progress of things we will decide about it.

Comment 8 by awdf@chromium.org, May 16 2018

Status: Assigned (was: Started)
Good plan - thanks. Have created Issue 843494 for ensuring test coverage, assigned to you.

I think the outcome of NotificationsTest.TestOriginPrefsNotSavedInIncognito should be that it's deleted in favour of non-content-setting-specific tests given the rule that incognito settings should not be persisted is not specific to notification settings.

Will keep this bug open & assigned to me for removing that test as such.

Comment 9 by awdf@chromium.org, May 16 2018

Labels: -Pri-1 Pri-3
I agree. OK.
Project Member

Comment 11 by chrome-privacy-bot@chromium.org, May 21 2018

Dear awdf,

This review has now been inactive for over 3 days. Could you please take appropriate steps to finish the review?

Your friendly privacy review bot.
Labels: -Review-Privacy
Project Member

Comment 13 by bugdroid1@chromium.org, May 29 2018

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

commit 7caf3a2eeb70cecaa0574fac1fa8f7569013982f
Author: Anita Woodruff <awdf@chromium.org>
Date: Tue May 29 16:18:57 2018

[Tests] Delete NotificationsTest.TestOriginPrefsNotSavedInIncognito

- This test was disabled long ago due to a crash, which no longer
occurs, however, the test is now out of date, and fails for unknown
reasons.

- Rather than attempt to fix up only this test for the incognito
persistence behaviour of notification content settings specifically,
it would be more appropriate to test the incognito persistence behaviour
of *all* content settings from content settings tests, since that's
where the logic lives. Filed https://crbug.com/843494 to ensure this
is tested more generally.

Bug:  841203 
Change-Id: I40ef6e73af449756c93e1afe8001c0dd9c9af1c2
Reviewed-on: https://chromium-review.googlesource.com/1065995
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562451}
[modify] https://crrev.com/7caf3a2eeb70cecaa0574fac1fa8f7569013982f/chrome/browser/notifications/notification_interactive_uitest.cc

Comment 14 by awdf@chromium.org, May 30 2018

Status: Fixed (was: Assigned)

Sign in to add a comment