Closed Bug 1644275 Opened 4 months ago Closed 3 months ago

Network panel shows successful requests as blocked

Categories

(DevTools :: Netmonitor, defect, P2)

defect

Tracking

(firefox-esr68 unaffected, firefox77 unaffected, firefox78 fixed, firefox79 fixed)

RESOLVED FIXED
Firefox 79
Tracking Status
firefox-esr68 --- unaffected
firefox77 --- unaffected
firefox78 --- fixed
firefox79 --- fixed

People

(Reporter: Harald, Assigned: bomsy, NeedInfo)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Attached image 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
 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
Has Regression Range: --- → yes
Priority: P2 → P3
Regressed by: 1555057
Summary: Network panel shows successful requests as blocked when served from Service Worker → Network panel shows successful requests as blocked

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

Flags: needinfo?(juhsu)

@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

Flags: needinfo?(bugmail)

@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

Has STR: --- → yes
Flags: needinfo?(hmanilla)
Priority: P3 → P2

@honza Oh yes! This is great!

Flags: needinfo?(hmanilla)
Attached image 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.

Assignee: nobody → hmanilla
Status: NEW → ASSIGNED
Flags: needinfo?(honzab.moz)

Hubert, what is the information you need from me?

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

@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))

Flags: needinfo?(juhsu)
Attached patch temp.patchSplinter Review

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

Flags: needinfo?(hmanilla)

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

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

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

Cool, will get this ready latest tomorrow.

Flags: needinfo?(hmanilla)
Attachment #9155668 - Attachment description: Bug 1644275 - Send create blocked request only if it is actually identified as blocked r=honza → Bug 1644275 - Add a new Channel map r=honza

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

Flags: needinfo?(honzab.moz)
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 79

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

See Also: → 1646940

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.

Attachment #9157814 - Attachment is obsolete: true

AIUI this is fixed for 78 by bug 1646940

Regressions: 1649771
You need to log in before you can comment on or make changes to this bug.