1644275 - Network panel shows successful requests as blocked

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[:Harald Kirschner :digitarald]

Attachment: image.png

What were you doing?

1. Open https://share.firefox.dev/3h6vl0B
2. Open Network panel
3. Refresh

What happened?

All requests show up as blocked, even though they loaded successfully.

What should have happened?

Hard reload works as expected. Soft reload shows most request as blocked (all requests that are served by the Service Worker).

In another example, https://firefox-devtools-waiting-sw.glitch.me/ serves /version from a Service Worker, but this is shown correctly.

Anything else we should know?

Browser Console throws this error 6 times when the requests are shown as blocked:

14:28:43.650 TypeError: can't access property "id", channel.loadInfo.browsingContext is null
    matchRequest resource://devtools/server/actors/network-monitor/network-observer.js:119
    _onRequestHeader resource://devtools/server/actors/network-monitor/network-observer.js:847
    observeActivity resource://devtools/server/actors/network-monitor/network-observer.js:622
    makeInfallible resource://devtools/shared/ThreadSafeDevToolsUtils.js:103
ThreadSafeDevToolsUtils.js:82:13
    reportException resource://devtools/shared/ThreadSafeDevToolsUtils.js:82
    makeInfallible resource://devtools/shared/ThreadSafeDevToolsUtils.js:109

Comment 1

[:Harald Kirschner :digitarald]

 9:47.05 INFO: Last good revision: 6e994c07eca1b3eec2c7b5ee59e37a7bc4452cbd
 9:47.05 INFO: First bad revision: a6237c79c2da071cdd13dc5691d91b2f5a39cbc1
 9:47.05 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6e994c07eca1b3eec2c7b5ee59e37a7bc4452cbd&tochange=a6237c79c2da071cdd13dc5691d91b2f5a39cbc1

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report Harald!

(In reply to :Harald Kirschner :digitarald from comment #0)

14:28:43.650 TypeError: can't access property "id", channel.loadInfo.browsingContext is null
    matchRequest resource://devtools/server/actors/network-monitor/network-observer.js:119
    _onRequestHeader resource://devtools/server/actors/network-monitor/network-observer.js:847
    observeActivity resource://devtools/server/actors/network-monitor/network-observer.js:622
    makeInfallible resource://devtools/shared/ThreadSafeDevToolsUtils.js:103
ThreadSafeDevToolsUtils.js:82:13
    reportException resource://devtools/shared/ThreadSafeDevToolsUtils.js:82
    makeInfallible resource://devtools/shared/ThreadSafeDevToolsUtils.js:109

This is unrelated and covered by bug 1643624

---

@junior: If you follow this STRs:

1. Open https://share.firefox.dev/3h6vl0B
2. Open Network panel
3. Refresh

... you'll see that "http-on-stop-request" is sent for _httpStopRequest https://profiler.firefox.com/before-load.js?__uncache=4%2F20%2F2020%2C%206%3A18%3A41%20PM

This request is created for the original https://profiler.firefox.com/before-load.js request that comes from a service worker. Is that expected?

Honza

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Andrew, please see my previous comment. I am seeing that requests coming from service-workers are using additional query param called __uncache, what is this for? It looks like this is for internal usage and it should not appear in the Network panel.

But, since "http-on-stop-request" is fired for them, they appear among list of other requests as blocked.

Is there a safe way how to ignore those?

(note that ignoring all "http-on-stop-request" events sent for requests that are having __uncache in the query string fixed this bug.)

There are also two places in the code base that feels hacky:
#1) Service workers never fire http-on-examine-cached-response?
https://searchfox.org/mozilla-central/rev/fac90408bcf52ca88a3dcd2ef30a379b68ab24e2/devtools/server/actors/network-monitor/network-observer.js#298

#2) Service worker requests emits cached-response notification on non-e10s, and not on e10s?
https://searchfox.org/mozilla-central/rev/fac90408bcf52ca88a3dcd2ef30a379b68ab24e2/devtools/server/actors/network-monitor/network-observer.js#461-462

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Bomsy, this use case can be used to reproduce the "duplicated requests" problem (aka multiple-selected, bug 1628162)

Can you try this on your machine?

Honza

Comment 5

[Hubert Boma Manilla (:bomsy)]

@honza Oh yes! This is great!

Comment 6

[Hubert Boma Manilla (:bomsy)]

Attachment: Fix_Image.png

I tried doing some investigations

diff --git a/devtools/server/actors/network-monitor/network-observer.js b/devtools/server/actors/network-monitor/network-observer.js
--- a/devtools/server/actors/network-monitor/network-observer.js
+++ b/devtools/server/actors/network-monitor/network-observer.js
@@ -363,10 +363,12 @@ NetworkObserver.prototype = {
       // If the owner isn't set we need to create the network event and send
       // it to the client. This happens in case where the request has been
       // blocked (e.g. CORS) and "http-on-stop-request" is the first notification.
-      this._createNetworkEvent(subject, {
-        blockedReason: reason,
-        blockingExtension: id,
-      });
+      if (id || reason) {
+        this._createNetworkEvent(subject, {
+          blockedReason: reason,
+          blockingExtension: id,
+        });
+      }
     }
   },

This seems to remove the duplicates, and also stops the blocking of the all the service worker requests.
Just for my clarifications

1. In this scenario the reason is 0 and the extension id is an empty string, which feels like it should not be blocking by default. Should we check for the availability of these before blocking?

Opening a patch for discussions and possible fix.

Comment 7

[Hubert Boma Manilla (:bomsy)]

Attachment: Bug 1644275 - Add a new Channel map r=honza

Comment 8

[Honza Bambas (:mayhemer)]

Hubert, what is the information you need from me?

Comment 9

[Hubert Boma Manilla (:bomsy)]

Sorry about that @Honza
The question from Comment 6. I just wanted to clarify the blocking scenario.

Comment 10

[Junior [inactive]]

@junior: If you follow this STRs:

1. Open https://share.firefox.dev/3h6vl0B
2. Open Network panel
3. Refresh

... you'll see that "http-on-stop-request" is sent for _httpStopRequest https://profiler.firefox.com/before-load.js?__uncache=4%2F20%2F2020%2C%206%3A18%3A41%20PM

This request is created for the original https://profiler.firefox.com/before-load.js request that comes from a service worker. Is that expected?

I think it's not expected.
The uncache query is from servcie worker: e=n+(i?"&":"?")+"__uncache="+encodeURIComponent(t))

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: temp.patch

I am attaching a patch we put together with @bomsy today.

It solves the issue by not using the channel object as a key in WeakMap - it isn't reliable.

The problem is that WeakMap can't use channel.channelId since it isn't an object and we can't use Map since we don't know when we should remove the entry in it (it's wrong to do it in 'onTransactionClose' since it doesn't have to be the last platform notification for a given channel).

We should use IterableWeakMap instead
https://github.com/tc39/proposal-weakrefs#iterable-weakmaps

FinalizationRegistry seems to be already supported
https://searchfox.org/mozilla-central/search?q=FinalizationRegistry&case=true&path=

@bomsy, can you please look at this?

Honza

Comment 12

[Julien Cristau [:jcristau]]

This sounds pretty bad. I don't suppose we can back the regressing changes out of beta for 78?

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Bomsy We should try to fix this in 79 and uplift to 78
Honza

Comment 14

[Julien Cristau [:jcristau]]

Note we're building the last beta for 78 this Friday.

Comment 15

[Hubert Boma Manilla (:bomsy)]

Cool, will get this ready latest tomorrow.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/615cd7d5434a
Add a new Channel map r=Honza

Comment 17

[Honza Bambas (:mayhemer)]

Is anything needed from me here? (Sorry for the delay to answer)

Comment 18

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/615cd7d5434a

Comment 19

[Hubert Boma Manilla (:bomsy)]

Attachment: Bug 1644275 - block request only when there is a reason or an extension id r=honza

Temp fix for uplift to 78, should undo for 79+

Comment 20

[Phabricator Automation]

Comment on attachment 9157814 [details]
Bug 1644275 - block request only when there is a reason or an extension id r=honza

Revision D80305 was moved to bug 1646940. Setting attachment 9157814 [details] to obsolete.

Comment 21

[Julien Cristau [:jcristau]]

AIUI this is fixed for 78 by bug 1646940