Closed Bug 1553849 Opened 6 years ago Closed 5 years ago

Implement "offline" mode for Network.emulateNetworkConditions

Categories

(Remote Protocol :: CDP, enhancement, P3)

enhancement

Tracking

(firefox78 fixed)

RESOLVED FIXED
Firefox 78
Tracking Status
firefox78 --- fixed

People

(Reporter: ochameau, Assigned: etienne)

References

Details

(Whiteboard: [puppeteer-beta-reserve])

Attachments

(1 file, 1 obsolete file)

This is used by one gutenberg test: change-detection.test.js.

Priority: -- → P2
Priority: P2 → P3
Whiteboard: [puppeteer-alp
Whiteboard: [puppeteer-alp → [puppeteer-alpha]
Priority: P3 → P2

This CDP endpoint is used in Puppeteer inside of NetworkManager.setOfflineMode(). It's used by Gutenberg in change-detection.test.js, and e2e-test-utils/src/offline-mode.js.

Priority: P2 → P3
Whiteboard: [puppeteer-alpha] → [puppeteer-alpha-reserve]
Priority: P3 → P2
Whiteboard: [puppeteer-alpha-reserve] → [puppeteer-beta-mvp]

Given that this isn't used in gutenberg test setup, I think we can consider it relatively low priority.

Priority: P2 → P3
Whiteboard: [puppeteer-beta-mvp] → [puppeteer-beta-reserve]

(In reply to Andrey Lushnikov from bug 1535110 comment #0)

CDP's Network.emulateNetworkConditions is widely used to emulate offline mode. Gecko's offline emulation could be probably done via nsIIOService service, but unfortunately it doesn't scope to a single tab.

How do we emulate offline mode on a per-tab basis?

The community of automation software Cypress does seem interested in this feature being implemented. Related Cypress Issue

Is there any workaround / another way at the moment to turn "offline mode" on without simulating mouse-clicks to the settings?

As someone who is new to Firefox development: which repository / directories / files would be affected by this? As someone unfamiliar with the code-base, it seems like exposing a new API command, routing it to a pre-existing internal feature - should be relatively doable? At least when it comes to the offline toggle.

I don't want to be wasting time and effort, but am available to help out.

Hi Etienne, if you'd like to give it a try here is some info to get started. Maybe you can see how well nsIIOService.offline works for your use case? I'm not familiar with per-tab offline settings.

First, see https://firefox-source-docs.mozilla.org/contributing/how_to_contribute_firefox.html

The commands are under <mozilla-central>/remote. In this case, the main changes would be in https://searchfox.org/mozilla-central/source/remote/domains/parent/Network.jsm
Tests under https://searchfox.org/mozilla-central/source/remote/test/browser/network and are run as ./mach test --headless remote/test/browser/network

Some developer documentation: https://firefox-source-docs.mozilla.org/remote/index.html and more at https://wiki.mozilla.org/Remote

Questions welcome.

I think if we are blocked on having per-tab offline settings it would still be fine to have them globally for now. We could limit the setting to a single tab later. Maybe worth asking devtools people, or checking code from devtools how it has been implemented there.

Seems to be simply setting nsIIOService.offline to true.

Note that it's not only offline which we need here, but also throttling. So if offline is most important maybe we file a new bug for that as starter.

@Maja thank you for the elaborate details - they are really helpful.
@Henrik sounds like a smart idea - although it might not hurt to have a "partial fix" related to this issue.

As for how devtools does this: devtools-frontend itself calls the internal function (Network.emulateNetworkConditions) within Chromium to handle this.

They internally keep track of a number of overrides.

In practice, they ignore uploadThroughput - I'm guessing it's there for future implementations. They cap the entire throughput using the value of downloadThroughput. They then expose these values through observer-patterns and OS-specific bindings. I'm afraid I lost the trail there. (I was not very helpful, I know)

@Maja thank you for your support.

Unfortunately, I am not familiar enough with the bindings / bridge between the JavaScript Modules and the "core" internals of Firefox. I have not been able to find something that is similar to the nsIIOService or XPCOM. It's probably there, but I fear I lack the appropriate knowledge of those parts. Lots of times a nice IDE can fill those gaps, but bridges between programming languages are not yet there.

(So to prevent people from waiting on me - I don't think I am able to make more progress.)

@Henrik after reading on the workflow of Firefox development and Bugzilla, with the automatic closing of issues - a separate Issue for just offline might indeed be desired. The issue https://bugzilla.mozilla.org/show_bug.cgi?id=1535110 could be reopened for that, perhaps..?

If offline mode would be important for you I can surely file a new bug and assist you to get this added. It shouldn't be too hard, and we can absolutely go ahead with setting it globally. Given your replies so far I'm sure it's something you can get done easily.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #1)

This CDP endpoint is used in Puppeteer inside of NetworkManager.setOfflineMode(). It's used by Gutenberg in change-detection.test.js, and e2e-test-utils/src/offline-mode.js.

And indeed Puppeteer only uses it for offline mode:
https://github.com/puppeteer/puppeteer/search?q=Network.emulateNetworkConditions&unscoped_q=Network.emulateNetworkConditions

As such lets just morph this bug to cover the offline mode.

Summary: Implement Network.emulateNetworkConditions → Implement "offline" mode for Network.emulateNetworkConditions

Related discussion: https://bugzilla.mozilla.org/show_bug.cgi?id=1140284#c23

Summary: Firefox allows access to localhost despite working offline, which does not match Chrome's behavior nor does it allow us to test it using puppeteer as easily.

Regarding the broader definition of Network.emulateNetworkConditions:

Do we have access to the consoleActor, which contains NetworkMonitor.throttleData?

In this example from the devtools, it seems very doable to simulate latency, as well as downloadSpeed and uploadSpeed. At least in the way people expect, by acting the same way as the devtools settings do.

(In reply to Etienne Bruines from comment #14)

Related discussion: https://bugzilla.mozilla.org/show_bug.cgi?id=1140284#c23

I have to note that this bug is something completely different (proxy related), and also marked as fixed 5 years ago. So there is no need to add more comments there.

Summary: Firefox allows access to localhost despite working offline, which does not match Chrome's behavior nor does it allow us to test it using puppeteer as easily.

Honza, is there a preference that would even allow us to stop allowing connections from localhost?

Flags: needinfo?(honzab.moz)

(In reply to Etienne Bruines from comment #15)

Regarding the broader definition of Network.emulateNetworkConditions:

Please lets keep this bug for offline support. I filed bug 1633831 for other possible additions.

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #16)

Honza, is there a preference that would even allow us to stop allowing connections from localhost?

Can you please be more specific what exactly you want?

Flags: needinfo?(honzab.moz) → needinfo?(hskupin)

(In reply to Honza Bambas (:mayhemer) from comment #18)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #16)

Honza, is there a preference that would even allow us to stop allowing connections from localhost?

Can you please be more specific what exactly you want?

At the moment, localhost is still accessible when Work Offline is enabled (either by pressing the button, or through remote APIs).

In situations where offline mode is activated to emulate offline environments (automated tests), it would be useful to not be able to connect to localhost either. Kinda defeats the whole purpose of simulating offline environments if the test server is still available.

We are currently looking for a way to make this possible, making localhost (and everything that resolves to it), adhere to the Work Offline setting.

Henrik, feel free to add anything to this.

(In reply to Honza Bambas (:mayhemer) from comment #18)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #16)

Honza, is there a preference that would even allow us to stop allowing connections from localhost?

Can you please be more specific what exactly you want?

Aha, you want to disallow ALSO connections TO localhost. OK. No, there is no preference for that.

(In reply to Honza Bambas (:mayhemer) from comment #20)

Aha, you want to disallow ALSO connections TO localhost. OK. No, there is no preference for that.

Or is there any other way like an interface of necko we can make use of to turn off localhost? If not what would be the best way to have it in the future? I don't mind filing a new bug. Thanks.

Flags: needinfo?(hskupin) → needinfo?(honzab.moz)

Thanks Honza! I filed bug 1634246 for that.

Etienne, lets continue with our implementation and get it landed. Whenever such an option to disable localhost is available we can come up with a follow-up patch.

Implements the Chrome Devtools Protocol (CDP) command
Network.emulateNetworkConditions partially. At the moment,
only "offline" is emulated, all other arguments are ignored.

Assignee: nobody → etienne
Status: NEW → ASSIGNED

It is difficult to test that "online" is actually online, because (as I've been told), the tests don't have access to the outside internet.

There's also some rather aggressive guard in place to ensure this:

 0:17.04 GECKO(49240) FATAL ERROR: Non-local network connections are disabled and a connection attempt to example.com (93.184.216.34) was made.
 0:17.04 GECKO(49240) You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.

Makes sense, so I don't really know what one would change about this. Asserting whether a connection was attempted and forcefully aborted? I don't know if one can try-catch fatal errors like that ;-).

But with the method of disabling proxy, I was able to verify that outside connections are not attempted while in offline-mode. It correctly shows the network offline error. This is being asserted.

(In reply to Etienne Bruines from comment #25)

Makes sense, so I don't really know what one would change about this. Asserting whether a connection was attempted and forcefully aborted? I don't know if one can try-catch fatal errors like that ;-).

To satisfy everyone's curiosity: no, you cannot simply try-catch fatal errors, they just stop the script immediately.

Attachment #9144582 - Attachment description: Bug 1553849 emulate offline CDP r?whimboo → Bug 1553849 - [remote] Add offline mode support to Network.emulateNetworkConditions r?whimboo
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b1c859fe04b1 [remote] Add offline mode support to Network.emulateNetworkConditions r=whimboo,remote-protocol-reviewers,maja_zf
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 78
Component: CDP: Network → CDP

Hi Etienne, we just got bug 1634246 fixed so that localhost connections should not be allowed when Firefox is in offline mode. I wonder if you would have the time to check that, and update the tests by removing the workaround (if possible) so that we can successfully test this. A new bug for that would be preferred. Thanks!

Since Bug 1634246 got resolved, connections to localhost can be
blocked when in offline mode. This commit now makes use of that
in the testing of the offline-part of emulateNetworkConditions.

Attachment #9240229 - Attachment description: Bug 1553849 - Remove workaround for localhost tests r?whimboo → Bug 1713775 - [remote] Remove workaround for localhost CDP browser chrome tests
Attachment #9240229 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: