New issue
Advanced search Search tips

Issue 756751 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug
Proj-Servicification



Sign in to add a comment

Some sync XHR tests are failing with mojo loading

Project Member Reported by yhirano@chromium.org, Aug 18 2017

Issue description

ResourceDispatcherHostBrowserTest.SyncXMLHttpRequest_Cancelled in content_browsertests
WebURLLoaderImplTest.SyncLengths in content_unittests

Please see https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/435214 and https://codereview.chromium.org/2553153002/#ps930001

Reilly, can you take a look?
 
Labels: OS-Linux
Status: Started (was: Assigned)
Attempting to reproduce locally. It is strange that this issue didn't pop up in the trybot run from when this patch landed:

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/430216
Only part of tests run with LoadingWithMojo enabled on try bots. I manually run all tests with LoadingWithMojo enabled to catch this kind of errors.
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 23 2017

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

commit 9a3a6ecc212a612f93ae905b45518e27cbbc1d78
Author: Reilly Grant <reillyg@chromium.org>
Date: Wed Aug 23 00:48:22 2017

Handle pipe closure in ThrottlingURLLoader

This change adds a connection error handler to the URLLoaderClient pipe
bound to the ThrottlingURLLoader. If this pipe unexpectedly closes
before the OnComplete message is received then the request is cancelled
with net::ERR_FAILED. This mirrors the behavior of ResourceDispatcher
when the ResourceHostMsg_SyncLoad IPC fails without returning an error
code.

This condition happens when a request is cancelled by the browser
process and the SyncResourceHandler or MojoAsyncResourceHandler is
unexpectedly destroyed.

Until PlzNavigate ships this failure can only be signaled for sync
requests as navigation loads expect to be able to be cancelled without
signaling any failure.

Bug:  756751 
Change-Id: I0ac8498ad38f27b50b64e1a13e6af2c6883b2f35
Reviewed-on: https://chromium-review.googlesource.com/621949
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Yuzhu Shen <yzshen@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496529}
[modify] https://crrev.com/9a3a6ecc212a612f93ae905b45518e27cbbc1d78/content/common/throttling_url_loader.cc
[modify] https://crrev.com/9a3a6ecc212a612f93ae905b45518e27cbbc1d78/content/common/throttling_url_loader.h
[modify] https://crrev.com/9a3a6ecc212a612f93ae905b45518e27cbbc1d78/content/common/throttling_url_loader_unittest.cc

Status: Fixed (was: Started)
Status: Assigned (was: Fixed)
How about WebURLLoaderImplTest.SyncLengths in content_unittests?
I'll take another look. I thought when I tested it that this change fixed that test as well.
Status: Fixed (was: Assigned)
$ ./out/Default/content_unittests --gtest_filter=WebURLLoaderImplTest.SyncLengths --enable-features=LoadingWithMojo --single-process-tests
Note: Google Test filter = WebURLLoaderImplTest.SyncLengths
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WebURLLoaderImplTest
[ RUN      ] WebURLLoaderImplTest.SyncLengths
[       OK ] WebURLLoaderImplTest.SyncLengths (0 ms)
[----------] 1 test from WebURLLoaderImplTest (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (0 ms total)
[  PASSED  ] 1 test.

Status: Assigned (was: Fixed)
Sorry for the confusion, but LoadingWithMojo is not directly controlled by the "LoadingWithMojo" feature. It is controlled by the "LoadingWithMojo" blink runtime enabled feature. They are connected by SetRuntimeFeaturesDefaultsAndUpdateFromArgs in runtime_features.cc but it seems not called in content_unittests.

So you ran the test with LoadingWithMojo disabled, I guess.

The test is still failing (see https://codereview.chromium.org/2553153002/#ps950001 and 
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/531657).
Status: Started (was: Assigned)
Updating RuntimeEnabledFeatures.json5 without also enabling features::kLoadingWithMojo puts the system into an inconsistent state. This particular test fails because enabling the Blink feature sets loading_ipc_type_ to kMojo but there is no logic in WebURLLoaderImplTest to initialize WebURLLoaderImpl with a valid URLLoaderFactory pointer.
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 26 2017

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

commit 4d219f851acf0c40ff6ae83105f59d70105c4226
Author: Reilly Grant <reillyg@chromium.org>
Date: Sat Aug 26 03:51:54 2017

Move sync XHR blocking back into ResourceDispatcher

Move the call to SyncLoadContext::StartAsyncWithWaitableEvent into
ResourceDispatcher so that the Mojo loading and legacy paths for a
synchronous XMLHttpRequest consistently go through the virtual LoadSync
method. This allows both to be overridden by TestResourceDispatcher in
the WebURLLoaderImplTests.

Bug:  756751 
Change-Id: I00ee27c287ff94be45969a6ba0aab59cccd3ca76
Reviewed-on: https://chromium-review.googlesource.com/634190
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497649}
[modify] https://crrev.com/4d219f851acf0c40ff6ae83105f59d70105c4226/content/child/resource_dispatcher.cc
[modify] https://crrev.com/4d219f851acf0c40ff6ae83105f59d70105c4226/content/child/resource_dispatcher.h
[modify] https://crrev.com/4d219f851acf0c40ff6ae83105f59d70105c4226/content/child/web_url_loader_impl.cc
[modify] https://crrev.com/4d219f851acf0c40ff6ae83105f59d70105c4226/content/child/web_url_loader_impl_unittest.cc

Status: Fixed (was: Started)
Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.
Project Member

Comment 13 by bugdroid1@chromium.org, Sep 18

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

commit c38237e6593750c763fa8a69308c37850b42f7a7
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Tue Sep 18 12:27:14 2018

ThrottlingURLLoader: Cleanup IsBrowserSideNavigationEnabled usages.

It's no longer useful following the launch of PlzNavigate.

Bug:  756751 , 789577
Change-Id: If342a766d03a832f6c333eccec970b4506faa847
Reviewed-on: https://chromium-review.googlesource.com/1230054
Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592008}
[modify] https://crrev.com/c38237e6593750c763fa8a69308c37850b42f7a7/content/common/throttling_url_loader.cc
[modify] https://crrev.com/c38237e6593750c763fa8a69308c37850b42f7a7/content/common/throttling_url_loader.h
[modify] https://crrev.com/c38237e6593750c763fa8a69308c37850b42f7a7/content/common/throttling_url_loader_unittest.cc

Sign in to add a comment