New issue
Advanced search Search tips

Issue 881538 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 7
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----



Sign in to add a comment

unit_tests failing on Android CFI

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Sep 6

Issue description

Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of jdonnelly@google.com

unit_tests has failed repeatedly on chromium.memory/Android CFI (runs 2793-2795 so far):

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI

The output on the build page doesn't list any failed steps:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI/2793

However, when I look at stdout, there are a bunch of "check failed" errors related to networking and URL loaders. I suspect https://crrev.com/c/1196563 may be at fault.
 
Description: Show this description
Cc: yhirano@chromium.org toyoshim@chromium.org
Owner: jdonnelly@chromium.org
Status: Started (was: Available)
The check failures, which each happen more than once across different tests are:

[FATAL:simple_watcher.cc(135)] Check failed: task_runner_->RunsTasksInCurrentSequence().
[FATAL:network_quality_estimator_manager.cc(119)] Check failed: (sequence_checker_).CalledOnValidSequence().
[FATAL:navigation_url_loader.cc(43)] Check failed: g_loader_factory == nullptr || factory == nullptr.
[FATAL:network_service.cc(134)] Check failed: !g_network_service.
[FATAL:stack_sampling_configuration.cc(97)] Check failed: PROFILE_FROM_COMMAND_LINE == configuration_ (3 vs. 0)

Two of those files are in src/services/network and one includes a file from that location. I'm going to go ahead and speculatively revert the two CLs in the blame list that affect code in src/services/network:
https://crrev.com/c/1196563
https://crrev.com/c/1203517
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 6

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

commit 4ff73ce8253b37f6d549219c5a9374cbee6095cb
Author: Justin Donnelly <jdonnelly@chromium.org>
Date: Thu Sep 06 21:38:23 2018

Revert "Remove cors:: namespace specifier in the namespace"

This reverts commit 0af66703ecaee2bb40fb0ec369e01c0efc3b4ef7.

Reason for revert: Suspected of causing check failures across a variety of tests on the Android CFI bot. See  https://crbug.com/881538  for more details.

Original change's description:
> Remove cors:: namespace specifier in the namespace
> 
> Bug: None
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: Ib487fee40beaac941b74c090d31133b12d61ea4a
> Reviewed-on: https://chromium-review.googlesource.com/1203517
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#589158}

TBR=toyoshim@chromium.org,yhirano@chromium.org

Change-Id: I1f7065dbeb733c4378babfba36236f2467073a5f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  881538 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1211949
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589315}
[modify] https://crrev.com/4ff73ce8253b37f6d549219c5a9374cbee6095cb/services/network/cors/cors_url_loader.cc
[modify] https://crrev.com/4ff73ce8253b37f6d549219c5a9374cbee6095cb/services/network/cors/preflight_controller.cc
[modify] https://crrev.com/4ff73ce8253b37f6d549219c5a9374cbee6095cb/services/network/cors/preflight_controller_unittest.cc
[modify] https://crrev.com/4ff73ce8253b37f6d549219c5a9374cbee6095cb/services/network/public/cpp/cors/cors_unittest.cc

Comment 4 Deleted

Looks like there is another, intermediate change that I need to revert: https://crrev.com/c/1203474.
Project Member

Comment 6 by bugdroid1@chromium.org, Sep 6

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

commit fb043b0b6a82e2e7ea325bd8a5b28c4a3fdbe3a9
Author: Justin Donnelly <jdonnelly@chromium.org>
Date: Thu Sep 06 21:55:50 2018

Revert "FetchManager should not set "referer" HTTP header"

This reverts commit bd27fde7c9069fccef4c571838ea25d9a1fa7acb.

Reason for revert: Suspected of causing check failures across a variety of tests on the Android CFI bot. See  https://crbug.com/881538  for more details.

Original change's description:
> FetchManager should not set "referer" HTTP header
> 
> Instead it should use ResourceRequest::SetReferrerString and
> ResourceRequest::SetReferrerPolicy.
> 
> Bug: 863769
> Change-Id: I4de47bae9aa259ace5ffb5bbdd17d6cf423931d7
> Reviewed-on: https://chromium-review.googlesource.com/1203474
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Dominic Farolino <domfarolino@gmail.com>
> Cr-Commit-Position: refs/heads/master@{#589159}

TBR=yhirano@chromium.org,domfarolino@gmail.com

Change-Id: Ie5e58f0e1b794acf2e9ca2f7d1ae24f043d9fe8e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 863769,  881538  
Reviewed-on: https://chromium-review.googlesource.com/1211957
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589318}
[modify] https://crrev.com/fb043b0b6a82e2e7ea325bd8a5b28c4a3fdbe3a9/third_party/blink/renderer/core/fetch/fetch_manager.cc
[modify] https://crrev.com/fb043b0b6a82e2e7ea325bd8a5b28c4a3fdbe3a9/third_party/blink/renderer/core/loader/threadable_loader.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 6

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

commit b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58
Author: Justin Donnelly <jdonnelly@chromium.org>
Date: Thu Sep 06 22:00:44 2018

Revert "Strengthen requirements on CORS-safelisted request-headers"

This reverts commit 074455deb4bfb5b117c641f65b60f32471093878.

Reason for revert: Suspected of causing check failures across a variety of tests on the Android CFI bot. See  https://crbug.com/881538  for more details.

Original change's description:
> Strengthen requirements on CORS-safelisted request-headers
> 
> With this CL, some request headers that used to be treated as
> CORS-safelisted are not CORS-safelisted any more. Specifically,
> 
>  - "accept", "accept-language" and "content-language" have a stronger
>    check on its value
>  - All headers whose value exceeds 128 bytes are treated as not
>    CORS-safelisted
>  - If the sum of value length of CORS-safelisted headers exceeds 1024,
>    then all of them are treated as not CORS-safelisted.
> 
> This CL also implements
> https://fetch.spec.whatwg.org/#no-cors-safelisted-request-header.
> 
> This is for https://github.com/whatwg/fetch/pull/736.
> 
> Bug:  824130 
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: Ib12a7dbff6367717a43130ae59304dca55b7bf4e
> Reviewed-on: https://chromium-review.googlesource.com/1196563
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#589153}

TBR=toyoshim@chromium.org,yhirano@chromium.org

Change-Id: I9952df291ff0aeaab0b50c6cff3de418b2272e71
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  824130 ,  881538  
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1211958
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589323}
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/services/network/cors/cors_url_loader.cc
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/services/network/cors/preflight_controller.cc
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/services/network/public/cpp/cors/cors.cc
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/services/network/public/cpp/cors/cors.h
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/services/network/public/cpp/cors/cors_unittest.cc
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/services/network/public/cpp/cors/preflight_result.cc
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/services/network/public/cpp/cors/preflight_result_unittest.cc
[add] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/third_party/WebKit/LayoutTests/external/wpt/fetch/api/cors/cors-preflight-not-cors-safelisted.any-expected.txt
[add] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/third_party/WebKit/LayoutTests/external/wpt/fetch/api/cors/cors-preflight-not-cors-safelisted.any.worker-expected.txt
[add] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/third_party/WebKit/LayoutTests/external/wpt/fetch/api/headers/headers-no-cors.window-expected.txt
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/third_party/blink/renderer/core/fetch/fetch_header_list.cc
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/third_party/blink/renderer/core/fetch/fetch_header_list.h
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/third_party/blink/renderer/core/fetch/fetch_header_list_test.cc
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/third_party/blink/renderer/core/fetch/headers.cc
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/third_party/blink/renderer/core/loader/threadable_loader.cc
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/third_party/blink/renderer/platform/loader/cors/cors.cc
[modify] https://crrev.com/b39baca0258fc15e772b4bf6d2d5f5bfef3a8a58/third_party/blink/renderer/platform/loader/cors/cors.h

It looks like all the tests passed in the latest run of the bot:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI/2796

Even though these reverts didn't land until the next run:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI/2797

Sorry for the trouble, I think it's safe to re-land these changes.
If the CLs are innocent can you please reland them?
I think it would be best if you relanded them so you can keep an eye on whether they succeed or fail. If I did it I'd just click the button and then forget about them as I'd be off to fight the next sheriffing fire.
Status: Fixed (was: Started)

Sign in to add a comment