Issue metadata
Sign in to add a comment
|
"ios_chrome_unittests" is flaky |
||||||||||||||||||||||||
Issue description"ios_chrome_unittests" is flaky. This issue was created automatically by the chromium-try-flakes app. Please find the right owner to fix the respective test/step and assign this issue to them. If the step/test is infrastructure-related, please add Infra-Troopers label and change issue status to Untriaged. When done, please remove the issue from Sheriff Bug Queue by removing the Sheriff-Chromium label. We have detected 8 recent flakes. List of all flakes can be found at https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyHwsSBUZsYWtlIhRpb3NfY2hyb21lX3VuaXR0ZXN0cww. This flaky test/step was previously tracked in issue 652639 .
,
Oct 28 2016
https://cs.chromium.org/chromium/src/components/security_interstitials/core/ssl_error_ui.cc?sq=package:chromium&dr=CSs&rcl=1477639132&l=94: ssl_errors::ErrorInfo error_info = ssl_errors::ErrorInfo::CreateError( ssl_errors::ErrorInfo::NetErrorToErrorType(cert_error_), ssl_info_.cert.get(), request_url_); Apparently cert_error_ sometimes has a value that triggers a broken code path. Must be that IDS_CERT_ERROR_UNABLE_TO_CHECK_REVOCATION_DETAILS is expected to have a $1 in it, and that https://cs.chromium.org/chromium/src/ui/base/l10n/l10n_util.cc?q=l10n_util.cc&sq=package:chromium&dr&l=711 blows up because it isn't there. I can't find that the string itself has been touched recently: https://chromium.googlesource.com/chromium/src/+blame/master/components/ssl_errors_strings.grdp, and none of the surrounding error messages have $1's in them, so I suppose the error was introduced elsewhere?
,
Oct 28 2016
Can't find any recent changes in the stack frames closest to the error, so I'm out of ideas. Disabling the test for now. The thing to investigate, I think, is why l10n thinks there should be a parameter to the string. I can't see from the code how this is possible. The fact that "Server's certificate cannot be checked" implies IDS_CERT_ERROR_UNABLE_TO_CHECK_REVOCATION_DESCRIPTION, which means the error should come from here: https://cs.chromium.org/chromium/src/components/ssl_errors/error_info.cc?sq=package:chromium&dr=CSs&rcl=1477639132&l=155 but both those methods invoke l10n_util::GetStringUTF16 with just one argument, i.e. no parameters. ccing felt@ who was last to change that part of the code.
,
Oct 28 2016
Assinging to eugenebut@ based off blame info for the test.
,
Oct 28 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/704f3cfe5a0278cf33eaa6bb5fe8389917a5e38a commit 704f3cfe5a0278cf33eaa6bb5fe8389917a5e38a Author: phoglund <phoglund@chromium.org> Date: Fri Oct 28 13:51:06 2016 Disabling flaky OverridableProceed test. BUG= 660343 TBR=eugenebut@chromium.org Review-Url: https://codereview.chromium.org/2462633002 Cr-Commit-Position: refs/heads/master@{#428356} [modify] https://crrev.com/704f3cfe5a0278cf33eaa6bb5fe8389917a5e38a/ios/chrome/browser/ssl/ios_ssl_error_handler_unittest.mm
,
Oct 28 2016
,
Oct 28 2016
According to the log this looks like a functionality problem, rather than a test bug. estark@, do you know who would be a good owner for this bug, while felt@ is out?
,
Oct 29 2016
Detected 20 new flakes for test/step "ios_chrome_unittests". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyHwsSBUZsYWtlIhRpb3NfY2hyb21lX3VuaXR0ZXN0cww. This message was posted automatically by the chromium-try-flakes app.
,
Oct 31 2016
Detected 6 new flakes for test/step "ios_chrome_unittests". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyHwsSBUZsYWtlIhRpb3NfY2hyb21lX3VuaXR0ZXN0cww. This message was posted automatically by the chromium-try-flakes app.
,
Oct 31 2016
I'm taking a look. Also adding meacer in case he has any ideas.
,
Oct 31 2016
Looking at the flake history, it looks like this might have started around Oct 25-26, with the following commits looking possibly very vaguely relevant. I'm working on reproducing locally. commit 87704da23bea0783ef0184a566398d5072154c41 Author: sdefresne <sdefresne@chromium.org> Date: Wed Oct 26 10:01:19 2016 -0700 Split //ios/chrome/browser in multiple targets. Create a new target for each sub-directory of src/ios/chrome/browser. Keep //ios/chrome/browser as a temporary target dependending on all of them to avoid breaking the roll. Remove some obsolete test files never used. BUG=658242 Review-Url: https://codereview.chromium.org/2443373002 Cr-Commit-Position: refs/heads/master@{#427721} commit 7a7fbd85e128e6bdea7d9acea82cbf29292aac36 Author: sdefresne <sdefresne@chromium.org> Date: Tue Oct 25 07:16:12 2016 -0700 Fix include path for components grit generated strings. BUG=None Review-Url: https://codereview.chromium.org/2448153002 Cr-Commit-Position: refs/heads/master@{#427341} commit 209dfc6b2338d707212ff264b662e90c91227051 Author: eugenebut <eugenebut@chromium.org> Date: Mon Oct 24 23:35:01 2016 -0700 [ios] Removed remaining code for URL spoofing interstitials. Chrome for iOS does not present URL spoofing error after switching to WKWebView. BUG=579697 Review-Url: https://codereview.chromium.org/2424643002 Cr-Commit-Position: refs/heads/master@{#427287}
,
Oct 31 2016
Thank you! cl/2424643002 cleans up unused code and resources. That CL could trigger the flakiness, but the problem is probably somewhere else. Sylvain, do you think your CLs could cause the flake?
,
Oct 31 2016
> Disabling the test for now. The thing to investigate, I think, is why l10n thinks there should be a parameter to the string. IDS_CERT_ERROR_UNABLE_TO_CHECK_REVOCATION_DETAILS does have a parameter though: https://cs.chromium.org/chromium/src/components/ssl_errors_strings.grdp?rcl=0&l=54 Shouldn't it be passed a hostname?
,
Oct 31 2016
I observe a very similar problem on the trunk on ios-simulator build bot (only on it). In my case, IDS_SSL_V2_PRIMARY_PARAGRAPH is equal to 15585 (while it should be 15586) and when it is used here https://cs.chromium.org/chromium/src/components/security_interstitials/core/ssl_error_ui.cc?sq=package:chromium&dr=CSs&rcl=1477926982&l=80 the "Your connection is not private" string (i.e. IDS_SSL_V2_HEADING, which is equal to 15585) is actually retrieved, which leads to a crash since the latter does not have a placeholder. This happens in IOSSSLErrorHandlerTest.NonOverridable test. Buildbot: https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/97873 I have been investigating this for a few days, because it prevents me from submitting my code (which is not related to iOS at all). As it was discussed with eugenebut@, I will try to find which of the CLs above causes the problem.
,
Oct 31 2016
The last time ios-buildbot passed IOSSSLErrorHandlerTest.NonOverridable on my CL was Tue Oct 25 2016 17:08:33 GMT-0700 (PDT): https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/94347 Then at Wed Oct 26 2016 18:18:13 GMT-0700 (PDT) it failed for the first time: https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/95321 I will continue the investigation.
,
Oct 31 2016
,
Nov 1 2016
I tried to revert 2443373002, but it required a merge, which looked tricky to me. However, I was able to make my CL pass ios-simulator build bot. Previously in my CL https://codereview.chromium.org/2360263002/ I removed an unused string from components/ntp_snippets_strings.grdp and when I added the string back, the CL passed the test.
,
Nov 1 2016
Looks a dependency issue, where some targets are not recompiled when the resource files change. Wouldn't clobbering the bot fix the issue?
,
Nov 1 2016
Detected 156 new flakes for test/step "ios_chrome_unittests". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyHwsSBUZsYWtlIhRpb3NfY2hyb21lX3VuaXR0ZXN0cww. This message was posted automatically by the chromium-try-flakes app.
,
Nov 1 2016
Adding Infra label based on comment 18.
,
Nov 2 2016
This is blocking https://codereview.chromium.org/2360263002/ from landing. Should that CL include a landmine to force a clobber?
,
Nov 2 2016
I guess the flakes are due to https://codereview.chromium.org/2429213007/ CL changing the grit resources ordering and missing dependencies. If my guess is correct, a land mine should fix this but should be landed separately.
,
Nov 2 2016
Incremental build bugs (such as missing dependencies) are not infra issues. Landmines are only a workaround. If you're aware of an issue, please work on making sure dependencies are correct.
,
Nov 2 2016
Assigning to thestig@ per comment #22
,
Nov 2 2016
Detected 16 new flakes for test/step "ios_chrome_unittests". To see the actual flakes, please visit https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=ahVzfmNocm9taXVtLXRyeS1mbGFrZXNyHwsSBUZsYWtlIhRpb3NfY2hyb21lX3VuaXR0ZXN0cww. This message was posted automatically by the chromium-try-flakes app.
,
Nov 2 2016
I can put in a landmine once I figure out how. However, I haven't built for iOS in a long time. Can someone else help figure out what the actual missing dependency is?
,
Nov 2 2016
Well, the landmine file says to use landmines as a last resort, so I'll start a public ios checkout and try to actually fix this.
,
Nov 3 2016
Unfortunately I cannot reproduce this. I synced to r427235, built ios_chrome_unittests, ran it, and it worked. I synced to r427236, repeated, and it still worked just fine. I followed the public build instructions and ran ios/build/tools/setup-gn.py. The only additional config I did was set up goma. I looked at the flaky dashboard and the first known flake started at r428008. So I'm syncing to that revision and giving that a try. Urge to landmine... raising.
,
Nov 3 2016
Landmine won't solve the issue. https://codereview.chromium.org/2464373002/ did add a landmine and the test still failed with the same error: [ RUN ] IOSSSLErrorHandlerTest.NonOverridable [21549:2567:1102/104545:18403329183548:WARNING:resource_bundle.cc(526)] locale resources are not loaded [21549:2567:1102/104545:18403329332500:WARNING:resource_bundle.cc(526)] locale resources are not loaded [21549:2567:1102/104545:18403345313603:WARNING:resource_bundle.cc(526)] locale resources are not loaded [21549:2567:1102/104545:18403345445363:WARNING:resource_bundle.cc(526)] locale resources are not loaded [21549:2567:1102/104545:18403346400707:WARNING:resource_bundle.cc(526)] locale resources are not loaded [21549:2567:1102/104545:18403346530398:WARNING:resource_bundle.cc(526)] locale resources are not loaded [21549:2567:1102/104545:18403346682989:WARNING:resource_bundle.cc(526)] locale resources are not loaded [21549:2567:1102/104545:18403347201663:FATAL:l10n_util.cc(711)] Check failed: std::string::npos != pos (18446744073709551615 vs. 18446744073709551615) Didn't find a $1 placeholder in https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/99207 So I think this is due to something else. Maybe the resources the test is trying to load is coming from a .pak file that is not repacked in the .pak used by the test itself. I'm surprise it is flaky though. Given the error I would have expected this to be reproducible at 100% (but maybe it is only flaky for incremental builds).
,
Nov 3 2016
So, the code that fails is the following:
load_time_data->SetString(
"primaryParagraph",
l10n_util::GetStringFUTF16(
IDS_SSL_V2_PRIMARY_PARAGRAPH,
common_string_util::GetFormattedHostName(request_url_)));
The resource is declared in components/security_interstitials_strings.grdp, so this means that gen/components/strings/components_strings_${locale}.pak files are not packed in ios_chrome_unittest.app paks.
$ find out/Debug-iphonesimulator/ios_chrome_unittests.app -name '*.pak'|wc -l
0
So it looks like there is no .pak file in ios_chrome_unittests.app, which explain why it is flaky on incremental builds and broken on clobber builds if the dependency was removed at some point. It turns out that it was removed by https://codereview.chromium.org/2443373002.
Fixing will just consist in adding by the dependency on "//ios/chrome/app/resources:packed_resources". Taking ownership of the bug.
,
Nov 3 2016
https://codereview.chromium.org/2467283005/ should fix the issue.
,
Nov 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b285099aa42c6be1526c77f33c93ea628fec5ab2 commit b285099aa42c6be1526c77f33c93ea628fec5ab2 Author: sdefresne <sdefresne@chromium.org> Date: Thu Nov 03 15:03:48 2016 Add missing dependency on packed resources to ios_chrome_unittests. The packed resources should be a dependency of //ios/chrome/app but due to the iOS downstream addition, it currently cannot be. Add the dependency to //ios/chrome:ios_chrome_unittests until a solution for packed resources has been implemented. The dependency was incorrectly removed by http://crrev.com/2443373002. BUG= 660343 Review-Url: https://codereview.chromium.org/2467283005 Cr-Commit-Position: refs/heads/master@{#429586} [modify] https://crrev.com/b285099aa42c6be1526c77f33c93ea628fec5ab2/ios/chrome/BUILD.gn
,
Nov 3 2016
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by phoglund@chromium.org
, Oct 28 2016