Issue metadata
Sign in to add a comment
|
Some sync XHR tests are failing with mojo loading |
||||||||||||||||||||||||
Issue descriptionResourceDispatcherHostBrowserTest.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?
,
Aug 21 2017
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.
,
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
,
Aug 23 2017
,
Aug 23 2017
How about WebURLLoaderImplTest.SyncLengths in content_unittests?
,
Aug 23 2017
I'll take another look. I thought when I tested it that this change fixed that test as well.
,
Aug 23 2017
$ ./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.
,
Aug 24 2017
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).
,
Aug 24 2017
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.
,
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
,
Aug 26 2017
,
Nov 7 2017
Apologies, applied the wrong component in bulk.
,
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 |
|||||||||||||||||||||||||
Comment 1 by reillyg@chromium.org
, Aug 18 2017Status: Started (was: Assigned)