Btw, I've also tried an onComplete listener. It doesn't fire either.

There are some experiments in 70 that may have caused a behavior change here but they are enabled a small percentage of the time.

chemo.b: can you check if this is still happening for you and provide information from chrome://version including the variations?

falken: hmm looks like it's working as expected now. Not sure if you still need my ://version info but I've attached it just in case.

Deleted: AboutVersion.html 4.8 KB

AboutVersion.html 4.8 KB View Download

Thanks, I wonder if this broke (and was fixed) for you due to the experiment going on then off.

Can you try the following?
1. Go to chrome://flags and enable #enable-service-worker-servicification.
2. Restart the browser.
3. See if this bug occurs or not.

After doing that, try also:
4. Go to chrome://flags and enable #network-service.
5. Restart the browser.
6. See if this bug occurs or not.

You probably would like to then go to chrome://flags and reset both to "Default" and restart.

Thanks for filing the issue.

@reporter: Could you please test the issue as per comment#6 and let us know your observations. Request you to confirm whether this issue can be closed in case the issue can be no longer seen.

Thanks.!

Sorry I've been a little busy. I'll check it out as soon as I have some spare time ;)

Thank you for providing more feedback. Adding the requester to the cc list.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Till then adding Needs-Feedback label as per comment#8, awaiting the reply from reporter.

Thanks!

This could have been related to turning on ServiceWorkerServicification. I'll look.

Hi chemo.b, I can't reproduce this even with ServiceWorkerServicification or NetworkService flags on, on Chrome 72. I'm using this simple extension:

background.js:
chrome.webRequest.onBeforeRequest.addListener(onBeforeRequest, {urls:["<all_urls>"]});

function onBeforeRequest(details) {
    console.log("onBeforeRequest()", details.url);
}

manifest.json:
{
    "manifest_version": 2,

    "name": "webrequest test",
    "description": "webrequest test",
    "version": "1.1.1.1",

    "background": {
        "scripts":["background.js"],
        "persistent": true
    },
    "permissions": [
        "webRequest",
        "*://*/*"
    ]
}

If I install that and navigate to some site, I see a lot of requests printed.

It also works on Chrome 70.

I put a test site at https://webrequest-test.glitch.me and then changed the addListener to:

chrome.webRequest.onBeforeRequest.addListener(onBeforeRequest, {urls:['*://webrequest-test.glitch.me/*']});

And the filtering seems to work too.

I haven't been able to reproduce the issue on my end either since oct 23rd. Weird. Anyway, looks like everything is fine (now)?

Thanks... did you already try the steps in https://bugs.chromium.org/p/chromium/issues/detail?id=897060#c6 to see if it repros with those flags?

@c#16: heh if I enable #enable-service-worker-servicification it immediately stops working properly.

Did you test this on v70? (you mentioned v72 but I don't run beta/canary).

It starts working as expected as soon as I disable it and restart Chrome.

I'll try a few times, see if I can figure out what causes this.

Nope, nothing on my end I (know I) can do to help pinpoint the issue. As soon as I enable the flag the callback for onBeforeRequest() doesn't get called.

When I enable #enable-service-worker-servicification onBeforeRequest() doesn't even seem to fire for the service worker now.

Note: I test this by creating a note in Keep with the URL that the background script is supposed to capture. Then I close all tabs except Keep. Then I click the link > Go to link. That opens it in a new tab.

If I have #enable-service-worker-servicification disabled:
onBeforeRequest is called, I open a "popup" window (chrome.windows.create()) and close the tab that was just opened.

If I have it enabled:
A new tab is opened, the link loads. onBeforeRequest doesn't get called.

Thanks for confirming-- I'll retest on Chrome 70 with enable-service-worker-servicification.

I still can't repro on Chrome 70 with #service-worker-servicification.

Can you try installing my extension and retrying with my demo site? If you can repro on that, then there may be a difference between our environments. If you can't, then there is a difference between our extensions or site.

Here is my extension source code (zip file is attached).

== background.js ==
chrome.webRequest.onBeforeRequest.addListener(onBeforeRequest, {urls:['*://webrequest-test.glitch.me/*']});

function onBeforeRequest(details) {
    console.log("onBeforeRequest()", details.url);
}

== manifest.json ==
{
    "manifest_version": 2,

    "name": "webrequest test",
    "description": "webrequest test",
    "version": "1.1.1.1",

    "background": {
        "scripts":["background.js"],
        "persistent": true
    },
    "permissions": [
        "webRequest",
        "*://*/*"
    ]
}


Here my repro steps:
- Download webrequest-test.zip.
- Unzip it somewhere.
- Launch Chrome
- In chrome://flags, verify #enable-service-worker-servicification is enabled. If not, enable it and then chrome://restart and repeat this step.
- In chrome://extensions, Load unpacked -> select the unziped directory.
- There should be a new extension "webrequest test". Click inspect views "background page". This opens a DevTools window
- Go to https://webrequest-test.glitch.me/
- In the console, look for output like "ready - please reload". Then reload.
- In the console, you should see output like "controlled! do your test".
- Look back at the DevTools window. If there is output for webrequest-test.glitch.me URLs, then the webRequest.onBeforeRequest is firing for the desired URLs.
- Try reloading the glitch.me tab and seeing if output appears or not. If not, then the event isn't firing.

Deleted: webrequest-test.zip 862 bytes

webrequest-test.zip 862 bytes Download

@#c20: I followed your steps and was not able to repro. I then tried my own extension and I still run into the same problem.

Then, I looked at the relevant lines in my bg.js and manifest:
(note: code below is simplified)

==background.js==
chrome.webRequest.onBeforeRequest.addListener(onBeforeRequest, {urls:['*://talkrrr.local/?*']});

function onBeforeRequest(details) {
    console.log("onBeforeRequest()", details.url);
}

==manifest.json==
"permissions": [
  // ..
  "webRequest",
  "*://talkrrr.rejh.nl/*",
  "*://talkrrr.local/*"
]

For some reason I thought I'd try modifying the code a little and registering the urls in background.js without the '?', so:
chrome.webRequest.onBeforeRequest.addListener(onBeforeRequest, {urls:['*://talkrrr.local/*']}); 

And then it works!

The downside is that it now fires for EVERY request on the domain and I really just need '/?*'

I guess I can filter out requests I don't care about, so no biggie. But isn't it kind of weird that enabling #service-worker-servicification causes this change in behaviour?

Thanks for confirming! Yes sounds strange.

Update: oh stupid me, I was cheering too early. I still don't see the request for https://talkrrr.local/

I do see:
/sw.js
/img/icons-app/small/ic_launcher-unread.png
/a-couple-of-other-image-resources

So.. it's still broken for me. Did you try my example code?
(try replacing local with rejh.nl in cfg.urls.app ;)

Waitaminute: does the SW in your example implement some form of caching? 

My SW uses Workbox-SW so most resources are served by the SW. Could it be that onBeforeRequest doesn't fire if the SW responds to the request instead of the network?

Ah, I see. You're right the SW doesn't use caching and now that I think of it, it doesn't even have a fetch event handler. That's probably a big difference.

If we don't even go to network, we probably don't go to onBeforeRequest in the service worker servicification architecture.

I've confirmed that when ServiceWorkerServicification is enabled and a service worker is installed with a fetch event handler, onBeforeRequest is not fired for requests from the page.

It's not even fired for requests that are re-issued by the service worker or when the service worker falls back, which should be fixable.

On the other hand, I'm not sure how to fix it to make it fire BEFORE the event reaches the service worker. ServiceWorkerServicification changes our architecture so the requests goes directly to the SW before it goes to the browser process or network service.

Can you help describe your use case more? Would it be sufficient to see requests that go to network, and not requests that the service worker responds with via the Cache Storage API?

Assign to bashi for making fetches *from* the SW go through extensions. To do this I think we need to call WillCreateURLLoaderFactory() in EmbeddedWorkerInstance's CreateFactoryBundle(), so the requests go through WebRequestProxyingURLLoaderFactory if needed. The test would be that respondWith(fetch()) and network fallback requests are visible by an extension.

I've also updated https://webrequest-test.glitch.me so you can use that with the extension in c#20 for local testing.

I was mistaken in c#26. When SWS is enabled but NS is disabled, the requests re-issued/falled back from the SW go to the extension. When NS is enabled, those requests skip the extension. A table of the number of onBeforeRequests fired for a request handled via:

                                  NS=off,SWS=off     NS=off,SWS=on   NS=on
network fallback                     1                    1            0
respondWith(fetch(request))          2                    1            0
respondWith(new Response('hi'))      1                    0            0

Note:
- new Response('hi') is effectively the same as returning a response from Cache Storage API.

@c#26: My extension is nothing more than a 'popup-window' shell for the web app. It puts a icon in the toolbar for quick access and makes sure that the extension popup opens when users click a link (or notification) to it (that's what the onBeforeRequest is for). 

The web app uses Workbox-SW to cache pretty much all the resources so the app loads from cache. The web app needs an internet connection so it's not really for offline availability, just faster loading speeds.

If the extension doesn't receive onBeforeRequest() events for cached resources I'd have to exclude the index.html from the cache. It wouldn't be a serious problem but it's not something I like ;)

c#30: thanks for the feedback! I think you may be also able to accomplish this behavior by responding with the cached resource while issuing an extra fetch() just so onBeforeRequest is fired. Something like the following (untested code):

self.addEventListener('fetch', async (event) => {
  event.respondWith(async () => {
    const response = cache.match(event.request);
    if (response) {
      // fetch() so onBeforeRequest sees it.
      event.waitUntil(fetch(event.request));
      return response;
    }
    return fetch(event.request);     
  }); 
});

c#27: Just started looking code and I noticed that WillCreateURLLoaderFactory() requires RenderFrameHost. I'm not sure how I can get a RenderFrameHost in CreateFactoryBundle.

+cduvall about this

Some code reading, it looks like the RFH is used sometimes to get a WebContents and sometimes to get a BrowserContext.

My guess is we can extend the code that needs WebContents to handle null WebContents. And we can plumb a BrowserContext from process_manager->browser_context() from EmbeddedWorkerInstance.

So WillCreateURLLoaderFactory() perhaps can take both a RFH (which can be null) and a BrowserContext in order to handle the case of a URLLoaderFactory created for a worker.

Thanks. I've just added another WillCreateURLLoaderFactoryForServiceWorker() which does the same. Looks like respondWith(fetch()) is hooked but network fallback isn't hooked. I'm not sure this is a right approach but my understanding is that this behavior is what we want for now.

Hm.. we'd want network fallback to go to the extension. And it'd be most correct if it's created as coming from the frame that made the original request.

Network fallback goes through the |fallback_factory|, which for frames comes from here in RenderFrameImpl::BuildServiceWorkerNetworkProviderForNavigation:

  scoped_refptr<network::SharedURLLoaderFactory> fallback_factory =
      network::SharedURLLoaderFactory::Create(
          GetLoaderFactoryBundle()->CloneWithoutDefaultFactory());

I'd expect the bundle from GetLoaderFactoryBundle() to be a hooked bundle from the browser. I wonder if cloning without default factory is creating a new bundle without the WebRequestProxyingURLLoaderFactory hook?

s/created as coming from the frame/treated as coming from the frame/

Ah, right. Network fallback is hooked when I change CloneWithoutDefaultFactory() to Clone(). Is it fine just using Clone() here? Or should we add a new clone method that bundles WebRequestProxyingURLLoaderFactory but not others?

We use CloneWithoutDefaultFactory() to ensure we skip AppCache which inserts itself as the default factory instead of network.

We'd want a way to CloneWithoutDefaultFactory() that goes to network but not AppCache while keeping WebRequestProxyingURLLoaderFactory. Would have to investigate... 

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

commit 09ee5e7e5b4fd3e0b99792e0539d67079a8b2932
Author: Kenichi Ishibashi <bashi@chromium.org>
Date: Tue Nov 27 07:12:38 2018

Make fetches from service worker go through webRequest API

Before this CL we didn't bundle WebRequestProxyingURLLoaderFactory
to the default URLLoaderFactory for service worker. This means that
fetches from service worker didn't go through webRequest extension
API when NetworkService is enabled. Bundle the factory so that
fetches from service worker go through extensions.

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Iabcc00674c8b62c9eae97e83f067d7a838a3d0fd
Bug:  897060 
Reviewed-on: https://chromium-review.googlesource.com/c/1333173
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611034}
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/android_webview/browser/aw_content_browser_client.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/android_webview/browser/aw_content_browser_client.h
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/chrome/browser/extensions/api/web_request/web_request_apitest.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/chrome/browser/extensions/service_worker_apitest.cc
[add] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/chrome/test/data/extensions/api_test/service_worker/webrequest/background.js
[add] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/chrome/test/data/extensions/api_test/service_worker/webrequest/hello.txt
[add] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/chrome/test/data/extensions/api_test/service_worker/webrequest/manifest.json
[add] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/chrome/test/data/extensions/api_test/service_worker/webrequest/page.js
[add] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/chrome/test/data/extensions/api_test/service_worker/webrequest/sw.js
[add] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/chrome/test/data/extensions/api_test/service_worker/webrequest/webpage.html
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/browser/service_worker/embedded_worker_instance.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/browser/shared_worker/shared_worker_host_unittest.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/common/url_loader_factory_bundle.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/common/url_loader_factory_bundle.h
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/public/browser/content_browser_client.h
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/renderer/loader/child_url_loader_factory_bundle.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/renderer/loader/child_url_loader_factory_bundle.h
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/renderer/loader/tracked_child_url_loader_factory_bundle.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/content/renderer/loader/tracked_child_url_loader_factory_bundle.h
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/extensions/browser/api/web_request/web_request_api.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/extensions/browser/api/web_request/web_request_api.h
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/extensions/shell/browser/shell_content_browser_client.cc
[modify] https://crrev.com/09ee5e7e5b4fd3e0b99792e0539d67079a8b2932/extensions/shell/browser/shell_content_browser_client.h

Thanks bashi@ for the fix. Just to be clear the expected change is:

(before the fix)
                                  NS=off,SWS=off     NS=off,SWS=on   NS=on
network fallback                     1                    1            0
respondWith(fetch(request))          2                    1            0
respondWith(new Response('hi'))      1                    0            0

(after the fix)
                                  NS=off,SWS=off     NS=off,SWS=on   NS=on
network fallback                     1                    1            1
respondWith(fetch(request))          2                    1            1
respondWith(new Response('hi'))      1                    0            0

i.e., there is still a difference from NS/SWS off and NS/SWS on, but SWS now matches NS. The requests that are intercepted by the SW don't fire web request API events, but requests made from the SW (fetch() or network fallback) do fire web request API events.

The fix only affects NS behavior, and I've been told a merge to 71 isn't necessary for NS. So changing Target to 72.

Setting WontFix. The plan is to keep the behavior as described: The requests that are intercepted by the SW don't fire web request API events, but requests made from the SW (fetch() or network fallback) do fire web request API events.