Closed Bug 1000540 Opened 10 years ago Closed 4 months ago

add an observer notification to nsFileChannel

Categories

(Core :: Networking: File, defect, P2)

Product:
Core
Component:
Networking: File
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
122 Branch
Tracking Flags:
Tracking Status
firefox123 --- fixed

People

(Reporter: jwalker, Assigned: dotoole)

References

(Depends on 1 open bug, Blocks 4 open bugs)

Details

(Keywords: DevAdvocacy, parity-chrome, Whiteboard: [polish-backlog][DevRel:P2])

Attachments

(1 file, 3 obsolete files)

Bug 1000540 - added an observer notification to nsFileChannel. r=valentin
48 bytes, text/x-phabricator-request
Details | Review 
I know, I know the clue is in the name "Net monitor", but many of the "what actually happened there" type questions that the netmonitor solves equally apply to file:// URLs.

I often test stuff just using a file:// URL to start with because it's a simple way to get started.
This is true. I thought we had bugs on file about our poor file:// support, but only came up with:

https://bugzilla.mozilla.org/show_bug.cgi?id=982606

There are some pretty important limitations with our network APIs that prevent us from really "monitoring" file URI accesses the way we do http. Hoping to get those things added with a redesign of the network channels APIs. [bug citation needed]
This was requested on StackOverflow at stackoverflow.com/questions/31379618
Keywords: DevAdvocacy
Whiteboard: [polish-backlog][chrome-parity]
Priority: -- → P2
Also relevant:

* Firefox allows XHR to the filesystem, but other browsers don't. So there is more reason for us to show file: traffic that other browsers.
* We also don't show data URIs, but could. That might be another bug though.
Aeons ago a similar request was created for Firebug:

https://github.com/firebug/firebug/issues/1224

Regarding the UI, it's important to make it clear that the files were loaded from the local file system instead of via a network request.

Sebastian
Whiteboard: [polish-backlog][chrome-parity] → [polish-backlog][chrome-parity][DevRel:P2]
So how soon it will be able to use?

It is really important for developing.
(In reply to lexxur from comment #7)
> So how soon it will be able to use?
> 
> It is really important for developing.

No one is actively working on this yet, so there is no estimation when this feature will be available. Though the importance of P2 and the DevAdvocacy keyword indicate that at some point someone from the DevTools team will pick this issue up.

Sebastian
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome
Whiteboard: [polish-backlog][chrome-parity][DevRel:P2] → [polish-backlog][DevRel:P2]
Product: Firefox → DevTools

Similarly, it seems local HTML files making remote network requests also show no activity which seemed surprising.

For example, this file has no network activity for me (Firefox 80.0 (64-bit) on Ubuntu 18.04).

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <link href="https://fonts.googleapis.com/css?family=Montserrat" rel="stylesheet" type="text/css">
</head>

<body>Hello</body>

</html>
See Also: → 1787445
Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 4 duplicates.
:Honza, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(odvarko)

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Flags: needinfo?(odvarko)
See Also: → 1193811

We might be able to reuse this old nsIWebProgressListener trick:
https://searchfox.org/mozilla-central/rev/88f285c5163f73abd209d4f73cfa476660351982/devtools/server/actors/webconsole/listeners/console-file-activity.js#71-104
But may be there is something better to do. This might only catch document requests and still ignore any other resource (JS, CSS, Images,...)?

Assignee: nobody → dotoole

Some information from the DevTools / BiDi point of view.

DevTools and BiDi both use the NetworkObserver helper in order to monitor network requests. This helper uses various observer notifications as well as activities (nsIHttpActivityObserver) to catch requests. For instance, we catch regular requests when we get the ACTIVITY_SUBTYPE_REQUEST_HEADER activity, meaning request headers should be available. And we catch some cached requests using the "http-on-examine-cached-response" observer notification. Other codepaths are used for service worker requests, early blocked requests, ...

They all end up creating a "network event" via #createNetworkEvent. This network event will be notified to the consumer, which will in exchange provide an owner object implementing several callbacks that will be called during the lifecycle of the network event or as we collect more information about it (eg addResponseStart, addResponseContent, addSecurityInfo).

So one option could be to modify the NetworkObserver to call #createNetworkEvent whenever a file:// request happens, but we might have issues with all the code which handles the lifecycle of the NetworkEvent after its' creation, as it normally assumes we have a nsIHttpChannel.

For devtools it might be a better idea to handle this at the NetworkEventWatcher level, which is basically the server part of devtools, responsible for translating platform information into something the DevTools UI can handle. This class currently consumes network events from the NetworkObserver and network event "resources" (around here). We even have a separate watcher for content process network events at devtools/server/actors/resources/network-events-content.js, which listens to different observers, but emits a similar payload.

So based on that I think we should try to have an observer notification for file requests and answer a few questions:

  • can we match the expected payload of those network event watchers (can we fill all the properties? etc...)
  • is it observable in the parent process or content process
  • can we know for which browsing context it was initiated

Note that typically the network events have 2 states for the DevTools UI: created and completed, but afterwards the client can ask for some additional information about the request.

All in all, the setup for network events in devtools is unnecessarily layered and complicated, so it would probably make sense to split the work between Necko and DevTools. If you can provide an observer notification we can try it out and see what would be the best path to implement this.

In case it was missed, I had a question on phabricator about the patch: https://phabricator.services.mozilla.com/D191317

Flags: needinfo?(dotoole)
Flags: needinfo?(dotoole)
Attachment #9359118 - Attachment description: WIP: Bug 1000540 - added fileChannel observer to allow netmonitor to display file urls. → Bug 1000540 - added fileChannel observer to allow netmonitor to display file urls.
Attachment #9359118 - Attachment description: Bug 1000540 - added fileChannel observer to allow netmonitor to display file urls. → WIP: Bug 1000540 - added fileChannel observer to allow netmonitor to display file urls.
Attachment #9359118 - Attachment description: WIP: Bug 1000540 - added fileChannel observer to allow netmonitor to display file urls. → Bug 1000540 - added fileChannel observer to allow netmonitor to display file urls.
Attachment #9359118 - Attachment description: Bug 1000540 - added fileChannel observer to allow netmonitor to display file urls. → Bug 1000540 - added fileChannel observer to allow netmonitor to display file urls. r=valentin,kershaw,bomsy
Attachment #9359118 - Attachment description: Bug 1000540 - added fileChannel observer to allow netmonitor to display file urls. r=valentin,kershaw,bomsy → Bug 1000540 - added fileChannel observer to netmonitor to display file urls. r=valentin,bomsy
Pushed by dotoole@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0d3e9db432a
added an observer notification to nsFileChannel. r=valentin,necko-reviewers

Backed out for causing build bustages in nsIPrincipal.h

  • Backout link
  • Push with failures
  • Failure Log
  • Failure line: /builds/worker/workspace/obj-build/dist/include/nsIPrincipal.h:358:20: error: inline function 'nsIPrincipal::IsSystemPrincipal' is not defined [-Werror,-Wundefined-inline]
    gmake[4]: *** [/builds/worker/checkouts/gecko/config/rules.mk:688: nsFileChannel.o] Error 1
Flags: needinfo?(dotoole)
Pushed by dotoole@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a5f11663cada
added an observer notification to nsFileChannel. r=valentin,necko-reviewers

https://hg.mozilla.org/mozilla-central/rev/a5f11663cada

Status: NEW → RESOLVED
Closed: 5 months ago
status-firefox121: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 121 Branch

Is this something we wanted to call out in the Fx121 relnotes? Please set the relnote-firefox: ? flag to nominate if yes.

We only landed the platform notification, not the UI. Dylan do you want to reopen the bug for the devtools part, or do you plan to move that patch to a new bug?

Status: RESOLVED → REOPENED
Flags: needinfo?(dotoole)
Resolution: FIXED → ---

Thanks for responding, yes I reopened it

Duplicate of this bug: 1866963
Duplicate of this bug: 1866961
Attachment #9367493 - Attachment is obsolete: true
Attachment #9359118 - Attachment description: Bug 1000540 - added fileChannel observer to netmonitor to display file urls. r=valentin,bomsy → Bug 1000540 - added fileChannel observer to netmonitor to display file urls. r=bomsy
Pushed by dotoole@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d6eab6d2a21
added mChannelId attribute to nsFileChannel. r=valentin,necko-reviewers

https://hg.mozilla.org/mozilla-central/rev/8d6eab6d2a21

Status: REOPENED → RESOLVED
Closed: 5 months ago → 5 months ago
status-firefox122: --- → fixed
Resolution: --- → FIXED

I think we still have some pending patches here.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Regressions: 1869851

Backed out for causing Bug 1869851

Backout link

Flags: needinfo?(dotoole)
Status: REOPENED → ASSIGNED
status-firefox121: fixed → ---
Target Milestone: 121 Branch → ---
Pushed by dotoole@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fee86ce779f5
added mChannelId attribute to nsFileChannel. r=valentin,necko-reviewers

Backed out for causing xpcshell failures on test_pause.js.
This only happened on Linux 18.04 x64 WebRender tsan opt.

[task 2023-12-14T22:47:08.099Z] 22:47:08     INFO -  TEST-START | tools/profiler/tests/xpcshell/test_pause.js
[task 2023-12-14T22:52:08.101Z] 22:52:08  WARNING -  TEST-UNEXPECTED-TIMEOUT | tools/profiler/tests/xpcshell/test_pause.js | Test timed out
[task 2023-12-14T22:52:08.102Z] 22:52:08     INFO -  TEST-INFO took 300000ms
[task 2023-12-14T22:52:08.102Z] 22:52:08     INFO -  >>>>>>>
[task 2023-12-14T22:52:08.102Z] 22:52:08     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2023-12-14T22:52:08.102Z] 22:52:08     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2023-12-14T22:52:08.102Z] 22:52:08     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2023-12-14T22:52:08.103Z] 22:52:08     INFO -  running event loop
[task 2023-12-14T22:52:08.103Z] 22:52:08     INFO -  tools/profiler/tests/xpcshell/test_pause.js | Starting
[task 2023-12-14T22:52:08.103Z] 22:52:08     INFO -  (xpcshell/head.js) | test pending (2)
[task 2023-12-14T22:52:08.103Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.104Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.104Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.104Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.104Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.105Z] 22:52:08     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2023-12-14T22:52:08.105Z] 22:52:08     INFO -  "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties"
[task 2023-12-14T22:52:08.105Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.105Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.105Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.106Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.106Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.106Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.106Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.107Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.107Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.107Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.107Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.107Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.108Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.109Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.109Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.109Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.109Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.109Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.110Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.110Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.110Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.110Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.110Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.111Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.111Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.111Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.111Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.111Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.112Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.112Z] 22:52:08     INFO -  TEST-PASS | tools/profiler/tests/xpcshell/test_pause.js |  - true == true
[task 2023-12-14T22:52:08.112Z] 22:52:08     INFO -  <<<<<<<
[task 2023-12-14T22:52:08.112Z] 22:52:08     INFO -  xpcshell return code: None
[task 2023-12-14T22:52:08.112Z] 22:52:08     INFO -  tools/profiler/tests/xpcshell/test_pause.js | Process still running after test!
[task 2023-12-14T22:52:09.139Z] 22:52:09     INFO -  INFO | Result summary:
[task 2023-12-14T22:52:09.140Z] 22:52:09     INFO -  INFO | Passed: 591
[task 2023-12-14T22:52:09.140Z] 22:52:09  WARNING -  INFO | Failed: 1
[task 2023-12-14T22:52:09.140Z] 22:52:09  WARNING -  One or more unittests failed.
[task 2023-12-14T22:52:09.141Z] 22:52:09     INFO -  INFO | Todo: 4
[task 2023-12-14T22:52:09.141Z] 22:52:09     INFO -  INFO | Retried: 1
[task 2023-12-14T22:52:09.144Z] 22:52:09     INFO -  SUITE-END | took 2308s
[task 2023-12-14T22:52:09.144Z] 22:52:09     INFO -  Node moz-http2 server shutting down ...
[task 2023-12-14T22:52:09.144Z] 22:52:09     INFO -  http3Server server shutting down ...
[task 2023-12-14T22:52:09.270Z] 22:52:09     INFO - Return code: 1
[task 2023-12-14T22:52:09.270Z] 22:52:09     INFO - TinderboxPrint: xpcshell-xpcshell<br/>591/<em class="testfail">1</em>/4
[task 2023-12-14T22:52:09.271Z] 22:52:09  WARNING - setting return code to 2
[task 2023-12-14T22:52:09.271Z] 22:52:09     INFO - The xpcshell suite: xpcshell ran with return status: FAILURE
Flags: needinfo?(dotoole)
Flags: needinfo?(dotoole)
See Also: → 1870293

Dylan: Since we already landed a first platform patch here, it becomes a bit hard to track the status of this work.
I think we should rather rename this bug to add an observer notification to nsFileChannel, close it, and handle the followup work in other bugs. Eg file a necko bug for "add mChannelId attribute to nsFileChannel", and another one for doing the DevTools UI work.

Status: ASSIGNED → RESOLVED
Closed: 5 months ago → 4 months ago
Component: Netmonitor → Networking: File
Product: DevTools → Core
Resolution: --- → FIXED
No longer duplicate of this bug: 1866961
No longer duplicate of this bug: 1866963
Summary: Netmonitor doesn't show resources loaded from a file url → add an observer notification to nsFileChannel
status-firefox123: --- → fixed
Target Milestone: --- → 122 Branch

Comment on attachment 9368099 [details]
Bug 1000540 - added mChannelId attribute to nsFileChannel. r=valentin

Revision D196137 was moved to bug 1870582. Setting attachment 9368099 [details] to obsolete.

Attachment #9368099 - Attachment is obsolete: true
Flags: needinfo?(dotoole)

Comment on attachment 9359118 [details]
Bug 1000540 - added fileChannel observer to netmonitor to display file urls. r=bomsy

Revision D191317 was moved to bug 1870580. Setting attachment 9359118 [details] to obsolete.

Attachment #9359118 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: