New issue
Advanced search Search tips

Issue 869302 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 9
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 846235



Sign in to add a comment

Pass layout tests when NetS13nSW is on

Project Member Reported by bashi@chromium.org, Jul 31

Issue description

Known failing test:

external/wpt/html/browsers/offline/appcache/workers/appcache-worker.https.html

We might have other failing tests.
 

Comment 1 Deleted

external/wpt/service-workers/service-worker/navigation-timing.https.html is also failing:

  FAIL Service worker controlled navigation timing network fallback assert_unreached: unexpected rejection: assert_true: Expected workerStart <= fetchStart expected true got false Reached unreachable code

Some other tests are failing on my machine but other failures are irrelevant to S13nServiceWorker.

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 9

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

commit 6579f5a480a41cda9992f67f1f6b0caffa2a3952
Author: Kenichi Ishibashi <bashi@chromium.org>
Date: Thu Aug 09 07:38:24 2018

S13nServiceWorker: Use RenderProcessHost's network factory for shared worker script loading

When S13nServiceWorker is on and NetworkService is off, we should
use content::URLLoaderFactoryImpl as the default network factory
of SharedWorkerScriptLoader because AppCacheRequestHandler wouldn't
work when NetworkService is off. It needs to use non-network
service path when a request is fallback to network to make sure
that an appcache is associated, if any.

SharedWorkerScriptLoader may ask the network factory to create
loaders more than once with the same |request_id| when there are
redirects. However, ResourceDispatcherHostImpl, which is used by
URLLoaderFactoryImpl, doesn't allow using the same |request_id|
multiple times. To work around this restriction we call
ResourceDispatcherHostImpl::CancellRequest() when a redirect happens
while loading a shared worker script.

This CL fixes following test when S13nServiceWorker is enabled:
- external/wpt/html/browsers/offline/appcache/workers/appcache-worker.https.html

Bug:  869302 
Change-Id: I5fae74439f82b3314f87a06bf9842db9df75a351
Reviewed-on: https://chromium-review.googlesource.com/1164741
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581818}
[modify] https://crrev.com/6579f5a480a41cda9992f67f1f6b0caffa2a3952/content/browser/shared_worker/shared_worker_host.cc
[modify] https://crrev.com/6579f5a480a41cda9992f67f1f6b0caffa2a3952/content/browser/shared_worker/shared_worker_script_loader.cc
[modify] https://crrev.com/6579f5a480a41cda9992f67f1f6b0caffa2a3952/content/browser/shared_worker/shared_worker_script_loader.h
[modify] https://crrev.com/6579f5a480a41cda9992f67f1f6b0caffa2a3952/content/browser/shared_worker/shared_worker_script_loader_factory.cc
[modify] https://crrev.com/6579f5a480a41cda9992f67f1f6b0caffa2a3952/content/browser/shared_worker/shared_worker_script_loader_factory.h
[modify] https://crrev.com/6579f5a480a41cda9992f67f1f6b0caffa2a3952/content/browser/shared_worker/shared_worker_service_impl.cc

According to https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/186262 currently the following tests are failing:


Unexpected Failures:
* external/wpt/content-security-policy/worker-src/shared-child.sub.html
* external/wpt/content-security-policy/worker-src/shared-fallback.sub.html
* external/wpt/content-security-policy/worker-src/shared-list.sub.html
* external/wpt/html/browsers/offline/appcache/workers/appcache-worker.https.html
* external/wpt/html/webappapis/the-windoworworkerglobalscope-mixin/Worker_Self_Origin.html
* external/wpt/workers/semantics/structured-clone/shared.html
* fast/events/message-port-transferables.html
* fast/workers/chromium/close-context-messageport-crash.html
* fast/workers/chromium/shared-worker-console-log.html
* http/tests/security/secureContexts/authenticated_worker.https.html
* http/tests/security/secureContexts/unauthenticated_worker.html
* virtual/mouseevent_fractional/fast/events/message-port-transferables.html
* virtual/user-activation-v2/fast/events/message-port-transferables.html

Regarding external/wpt/content-security-policy/worker-src/shared-child.sub.html, a shared worker in the test requires the "MojoBlobURLs" flag to handle a Blob URL. See SharedWorker::Create():
https://chromium.googlesource.com/chromium/src/+/c260c9a88bad63450919aa5f953dcbe8f0b874d4/third_party/blink/renderer/core/workers/shared_worker.cc#86

The NetworkService flag enables it, but the S13nServiceWorker doesn't. That's the reason why the test fails when only the S13nServiceWorker flag is enabled.
Labels: -Pri-3 M-71 Pri-1
Thank you for the investigation, hiroki-san! Enabling MojoBlobURLs makes pass all tests except for appcache-worker.https.html.

Prioritizing this so that we can enable S13nServiceWorker by default.
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 25

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

commit 53d0dd69a1def0eb6346a40f86b6ebae7d9ee5db
Author: Kenichi Ishibashi <bashi@chromium.org>
Date: Tue Sep 25 05:14:46 2018

S13nServiceWorker: Use RenderProcessHost's URLLoaderFactory as default factory for shared worker

When S13nServiceWorker is on but NetworkService is off, we want to use
URLLoaderFactory from RenderProcessHost as AppCacheReqeustHandler
wouldn't work when NetworkService is off.

Before this CL we used RenderProcessHost's URLLoaderFactory only for
subresources. This CL changes to use it for main resources as well.
This fixes
external/wpt/html/browsers/offline/appcache/workers/appcache-worker.https.html.

Bug:  869302 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I7ad8c840ac9b6424e102f09cf43a146f9f858c60
Reviewed-on: https://chromium-review.googlesource.com/1229720
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593826}
[modify] https://crrev.com/53d0dd69a1def0eb6346a40f86b6ebae7d9ee5db/content/browser/shared_worker/shared_worker_host.cc
[modify] https://crrev.com/53d0dd69a1def0eb6346a40f86b6ebae7d9ee5db/content/browser/shared_worker/shared_worker_service_impl.cc

Status: Fixed (was: Assigned)
Looks like all layout tests are passing for now (though there are some failures on content_browsertests).
https://chromium-review.googlesource.com/c/chromium/src/+/1270615/1

I filed a bug for content_browsertests failures:  crbug.com/893476 

Let me close this. I'll file a new bug when we see another regression in the future.

Sign in to add a comment