1140284 - Can not connect to localhost after proxy change in offline mode

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[Junior [inactive]]

STR
* browser
1. Use File -> "Offline Mode" to move to offline mode
2. Click Preference -> Advanced -> Network ->Cached Web Content -> Clear Now
3. Use Preference -> Advanced -> Network -> Connection -> Settings
   to change proxy to anyone different
4. Try to make a connection to 127.0.0.1/localhost through http

Actual result:
Show Offline mode

Expected result:
Try to make a connection if no http service is in localhost
Or show the page if localhost has a http service.

* Firefox OS
1. Enter Airplane mode
2. Use webIDE to change "network.proxy.type" to a different value
3. Use browser to make a connection to 127.0.0.1/localhost


Actual result:
Show "http://127.0.0.1 requires an Internet connection."

Expected result:
Show "A network error occurred while trying to reach the site."

Comment 1

[Junior [inactive]]

Actually Shih-Chiang and I have found the root cause, and we need some suggestion.

The thing is changing proxy will restart the DNSService
https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsDNSService2.cpp#592
https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsDNSService2.cpp#964

However, becaouse of offline, the initialization bails out before nsHostResolver is created.
https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsDNSService2.cpp#608

Therefore, DNSService.asyncResolve no longer works even if we try to connect to localhost
https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsDNSService2.cpp#758

Hello Patrick and Ehsan:
Do we have risk if we don't check the online/offline when DNSService is initialing? 
We have done some experiment and behavior looks good.
Just push to try if anything is out of expectation:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c98009b80da5

Or any suggestion to step advance?

Comment 2

[Patrick McManus [:mcmanus]]

daniel is becoming master of on/offline.

Comment 3

[Daniel Stenberg [:bagder]]

Isn't this just a version of the good old Bug 339814?

Comment 4

[(no longer active)]

(In reply to Daniel Stenberg [:bagder] from comment #3)
> Isn't this just a version of the good old Bug 339814?

Yes, but comment 1 seems to have some concrete info on what causes it!

Comment 5

[Patrick McManus [:mcmanus]]

daniel, can you take it?

Comment 6

[Daniel Stenberg [:bagder]]

Attachment: 0001-DNS-only-flush-cache-not-restart-on-new-proxy-type.patch

I think we can avoid the Shutdown()/Init() loop for this case and only flush the caches? This seems to not make the problem trigger for me.

An alternative solution would be allow the Init() method to fire up the resolver even when in offline mode - and we might want that anyway for other prefs changes that I believe still need to restart the resolvers to take effect but yet we want it to work while offline.

Comment 7

[Daniel Stenberg [:bagder]]

The patch that introduced the delayed start of the resolver comes from Bug 614286 (by :ehsan), but unfortunately the discussion there mostly focuses on the socket transport service.

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Theoretically we might switch to offline mode before the first initialization of nsDNSService. In this case, we'll still the same issue if we don't change the logic in https://dxr.mozilla.org/mozilla-central/source/netwerk/dns/nsDNSService2.cpp#608.

Comment 9

[Daniel Stenberg [:bagder]]

Good point, I did a little experiment with simply removing the check and starting the resolver unconditionally, which seemed to work fine in my manual tests but causes several test cases to fail:

--- a/netwerk/dns/nsDNSService2.cpp
+++ b/netwerk/dns/nsDNSService2.cpp
@@ -601,15 +601,10 @@ nsDNSService::Init()
 
     }
 
     nsDNSPrefetch::Initialize(this);
 
-    // Don't initialize the resolver if we're in offline mode.
-    // Later on, the IO service will reinitialize us when going online.
-    if (gIOService->IsOffline() && !gIOService->IsComingOnline())
-        return NS_OK;
-
     nsCOMPtr<nsIIDNService> idn = do_GetService(NS_IDNSERVICE_CONTRACTID);
 
     nsCOMPtr<nsIObserverService> obs =
         do_GetService(NS_OBSERVERSERVICE_CONTRACTID);

Comment 10

[Daniel Stenberg [:bagder]]

Attachment: 0001-DNS-start-the-resolver-even-when-offline.patch

Here's the patch that starts the resolver unconditionally, even if offline, and the try-run for this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f33b01f1a71

It also seems to fix the case when I tried it manually.

It is just a proof of concept still, could use some further cleaning up if we decide to go this path.

Comment 11

[Daniel Stenberg [:bagder]]

Ehsan, do you recall the details on why that removal of the unconditional start of the resolver as it seems bringing that back fixes this problem (and not unlikely for bug 339814 too).

Comment 12

[(no longer active)]

(In reply to Daniel Stenberg [:bagder] from comment #11)
> Ehsan, do you recall the details on why that removal of the unconditional
> start of the resolver as it seems bringing that back fixes this problem (and
> not unlikely for bug 339814 too).

I made that change in bug 614286 because we used to crash if attempting to initialize this stuff before the STS.

Comment 13

[Daniel Stenberg [:bagder]]

Attachment: v2-0001-Bug-1140284-start-the-resolver-even-when-offline-.patch

Here's an official patch suggestion then. You OK with reviewing :ehsan - since you did this change before I figured you might be a good pair of eyes on this.

This patch starts the resolver even when when offline, to make 'localhost' still work.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42ea2c7b3f6a

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42ea2c7b3f6a

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8575979 [details] [diff] [review]
v2-0001-Bug-1140284-start-the-resolver-even-when-offline-.patch

Note that this is actually desired for v2.1s and v2.0m; I'm not sure how this impacts landing flags and branches.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Not a regression.

User impact if declined: 
v2.1s and v2.0m really want this fix.  See bug 1169474 and/or bug 1169401 for more details.

Testing completed:
Trunk is happy, v2.1 and v2.0 are sad and the change is very simple.

Risk to taking this patch (and alternatives if risky):
No risk perceived.  The resolver service will initialize at startup (or on demand) when offline when it previously would not.  This is harmless and explicitly desired.
 
String or UUID changes made by this patch:
No changes.

Comment 18

[Josh Cheng [:josh]]

Comment on attachment 8575979 [details] [diff] [review]
v2-0001-Bug-1140284-start-the-resolver-even-when-offline-.patch

Hi Ryan, 
I think this worth to be land on 2.0/2.1 and then cherry pick to 2.0M/2.1S. How do you think? :)

Comment 19

[Ryan VanderMeulen [:RyanVM]]

I assume you want this on v2.2 as well.

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cc5b15c969ff
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/1d52d525d256
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/6a1e2bff9fe0

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/1d52d525d256

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/6a1e2bff9fe0

Comment 23

[Etienne Bruines]

This behavior does not match the behavior of Chrome, when it comes to emulating offline network conditions.

This causes some issues with the puppeteer-tests within gecko/remote/test/puppeteer when emulating network conditions in automated tests. https://bugzilla.mozilla.org/show_bug.cgi?id=1553849

    // gecko/remote/test/puppeteer/test/page.spec.js:265
    it('should work', async({page, server}) => {
      await page.setOfflineMode(true);
      let error = null;
      await page.goto(server.EMPTY_PAGE).catch(e => error = e); // = http://localhost:12345/empty.html
      // no error is thrown, because it is accessible and "just works - and that's a bad thing here
      expect(error).toBeTruthy();
      await page.setOfflineMode(false);
      const response = await page.reload();
      expect(response.status()).toBe(200);
    });

Related issues also seemed to have allowed navigating to everything that "resolves" to localhost.

Two main questions that arise:

1. Should the Firefox behavior match that of Chromium?
2. What about emulating network conditions to test how websites behave when offline? Most "test servers" in CI environments will undoubtedly run on localhost - it would go a long way in automated testing of Progressive Web Apps (PWAs).

As to create a workable situation, is there a configuration value that can be set somewhere to disallow access to localhost when Work Offline is set?