The default bug view has changed. See this FAQ.

Conn: Offline: localhost/127.0.0.1 not accessible

RESOLVED FIXED in mozilla18

Status

()

Core
Networking
P3
normal
RESOLVED FIXED
16 years ago
2 years ago

People

(Reporter: Malx, Assigned: hobophobe)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 14 obsolete attachments)

3.33 KB, patch
Details | Diff | Splinter Review
36.35 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

16 years ago
It's suppose, that you set "offline" if you are not connected to Internet.
But still, as a developer, you could have local WebServer - and you still
can't access it!!! It's a bug IMHO.
 More for this - PSM, Help also acts as a local web server jut to display
it's help pages. So you firbids to see help in "offline" mode for _every_
user.
See also bug about cache/offline.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

16 years ago
Priority: -- → P3
Target Milestone: --- → mozilla0.9.3

Comment 1

16 years ago
I can think of arguments for this both ways. Can someone post to the newsgroup
and moderate this?
Summary: localhost/127.0.0.1 not accessible in OFFLINE mode → Conn: localhost/127.0.0.1 not accessible in OFFLINE mode

Comment 2

16 years ago
127.0.0.1 shouldn't work in offline mode, but localhost might.
(Reporter)

Comment 3

16 years ago
Don't you thinks this is same?! :)
Anyway - PSM Help uses "127.0.0.1"
You could see it by clicking on HELP in PSM and then close
the window appears before it loads HTML page inside.

Then new window opens - and it is normal Mozilla window so you could see
URL yourself.

Comment 4

16 years ago
If they are the same, then this bug is invalid. 127.0.0.1 is used for loopback
(Reporter)

Comment 5

16 years ago
Sorry, could you explain?
127.0.0.1 - is used for loopback. To connect to [current] network device.
localhost is just a name. through "/etc/hosts" file it converts to 
127.0.0.1 (no IP packets use names , only IPs ;)

And the main reason - "HELP of PSM is not working in 'offline' mode" :)
I'd also like to see http://localhost/ accessible in Offline mode but just 
making an exception for "localhost" and 127.0.0.1 is the wrong way to go IMO: 
I'm running an offline Apache webserver and installed further virtual hosts (in 
my case simple subdomains) such as "info.localhost", but these could be 
completely different names. The best way would be to allow making a list of 
URLs that even run in Offline mode, maybe in prefs.js?

So if you just want to make Help available in Offline mode, please don't use 
that bug but file another one.

Comment 7

16 years ago
basic:
I think that it would make more sense to argue the opposite. 127.0.0.1 is the
"loopback" address, and it basically should always be on if TCP/IP is on.
"localhost" is a convential name for 127.0.0.1. It would not make a lot of sense
to let localhost work but not 127.0.0.1.

malvin:
If Mozilla is dependent on seeing PSM on the loopback address, then this might
be one of those "lets argue about it because there might be something I haven't
thought of" points.

Jens:
Selectable off-line "zones"/"realms" (list of domains or URLs) is probably a
good RFE. It would solve this problem, but it should probably be a new bug.
Create a new bug if this doesn't exist already. You feature request might go
well with what I've proposed in bug 88218.

For your specific "I want to run virtual domains over 127.0.0.1", I'm not sure
what hacked hosts file can't do that offline does.

Updated

16 years ago
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Updated

16 years ago
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Updated

16 years ago
Target Milestone: mozilla0.9.5 → mozilla0.9.6

Updated

16 years ago
Target Milestone: mozilla0.9.6 → mozilla1.0

Comment 8

16 years ago
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)
Target Milestone: mozilla1.0 → mozilla1.0.1

Updated

15 years ago
Target Milestone: mozilla1.0.1 → Future

Comment 9

15 years ago
neeti:
What is the expected behavior here? 

I think this should be RESOLVED/WFM, because offline takes the application
offline. If we want to make the Offline mode more network-level disconnect
friendly, we basically need to implement bug 88218.
Blocks: 88218
Keywords: helpwanted
Summary: Conn: localhost/127.0.0.1 not accessible in OFFLINE mode → Conn: Offline: localhost/127.0.0.1 not accessible

Comment 10

15 years ago
moving neeti's futured bugs for triaging.
Assignee: neeti → new-network-bugs

Comment 11

14 years ago
Maybe this is tied into bug 164802
[MozillaFirebird] v 0.6.1

http://localhost:8080 
goes to http://www.localhost.net.au/
http://127.0.0.0:8080
can not be found.

Under Mozilla 1.5a
http://localhost:8080 and http://127.0.0.0:8080
both bring up tomcats front page


When I set my firwall to disallow internet connections
it says it cannot reach www.google.com.
It should never do this search because I am running tomcat
on that port !

I brought my browser settings over form Mozilla 1.4 final,
and I --hate-- keyword/search completion, so this was turned
off. Now I can find no way to turn this feature off.
Depends on: 164802

Comment 12

14 years ago
-> defaults:

This scenario affects alot of our higher-end users who are offline.

Robert, see the bug you referenced for more info. That has nothing to do w/
going offline.
Assignee: new-network-bugs → darin

Comment 13

13 years ago
fixed dependency.
Depends on: 169106
No longer depends on: 164802

Updated

11 years ago
Assignee: darin → nobody
QA Contact: benc → networking
Target Milestone: Future → ---
Blocks: 339814

Comment 14

9 years ago
Hello, Im using Firefox 3.0.1 and Windows XP and I use a lot local websites with no internet connection. I have the Offline problem all the time and I have to uncheck the work offline option. But everytime I restart Firefox I have to do the same again.

I tried with localhost and with 127.0.0.1 and everytime happens the same. Why this option is not saved and how I can solve this problem? Is that a bug?

Updated

9 years ago
Blocks: 426487

Comment 15

9 years ago
New behaviour of the 'work offline' feature changes the degree of annoyance of this bug.  Firefox now tries to detect when you are offline and starts in offline mode.  Now, users who need to access local web servers cannot do so without unchecking "work offline", because the auto-detection may have disabled their access to "localhost".

Updated

7 years ago
Duplicate of this bug: 531024
Duplicate of this bug: 432434

Comment 18

7 years ago
Another use case: the R online help (see http://www.r-project.org/ about R) does open help pages in browser, and functions as a http server to provide these pages, probably from some compressed representation. The opened URL refers to 127.0.0.1:<some high port number>. Due to this bug, users will only get an error message when they try to open the help, which can be confusing indeed. The correct solution of unckecking the offline mode menu item isn't obvious enough.

Updated

7 years ago

Updated

7 years ago
Blocks: 565564
Blocks: 620472
Blocks: 2892
(Assignee)

Comment 19

6 years ago
Created attachment 573692 [details] [diff] [review]
Implements offline-mode with respect for loopback.

This doesn't give network zones, but it does rework offline mode to let loopback stay live.  

It's not completely network-silent, as DNS requests are still attempted (and succeed if the network is available).  Suggestions welcome.  

The changes:

 + nsIOService no longer shuts down DNS and the socket service when going offline, it only does that on shutdown.  Needs a few more changes, about what observers to notify and when.

 + nsHttpChannel no longer allows only cached pages to load.  Non-local pages in cache should still load from cache.

 + nsSocketTransport2 will now balk (with NS_ERROR_OFFLINE) if offline and the address isn't loopback.

 + nsHttpHandler now observes the NS_IOSERVICE_GOING_OFFLINE_TOPIC and clears existing connections (otherwise, after going offline you could load pages only from domains with reusable connections).  This was not needed for FTP, as it already did that.  Not sure, but probably will be needed for websockets?

 + NSPRPUB's PR_IsNetAddrType() needed to modify its checks for IPv4 loopback prefix rather than for just the typical address of 127.0.0.1
(Assignee)

Comment 20

5 years ago
Created attachment 575552 [details] [diff] [review]
netwerk: single check for observer service before notifying
Attachment #573692 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Created attachment 575555 [details] [diff] [review]
NSPR: PR_IsNetAddrType should check for IPv4 loopback as the entire 127/8 instead of just 127.0.0.1

I split this out into a separate patch, as NSPR is a separate product.
(Assignee)

Comment 22

5 years ago
Created attachment 575570 [details] [diff] [review]
NSPR: Add a pair of tests (one IPv4, one IPv6 with IPv4 mapped address)
Attachment #575555 - Attachment is obsolete: true
Adam: You have to request reviews from someone !
Comment on attachment 575552 [details] [diff] [review]
netwerk: single check for observer service before notifying

Jason chosen by random selection.
Attachment #575552 - Flags: review?(jduell.mcbugs)
Comment on attachment 575552 [details] [diff] [review]
netwerk: single check for observer service before notifying

Review of attachment 575552 [details] [diff] [review]:
-----------------------------------------------------------------

I'll steal the review, because there is at least one significant problem.. 

but, first, thanks Adam for the patch!

As you note in comment 19 this no longer shuts down the socket service when going offline. Shutting that down actually results in detaching all the active sockets which is something some subsystems are going to depend on. You do add code to close idle connections in http connection manager, but that's just a subset of things. For instance active sockets might just hang  - this includes spdy sockets which are always 'active' and as you note websockets too. Also anything using the socket api - such as maybe IMAP in Thunderbird probably has this implicit assumption. So I would say that minimally 

a] all the sockets should be detached
b] dns should have an offline mode where it only resolves literals and (bonus points) cache hits.
Attachment #575552 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 26

5 years ago
Created attachment 619220 [details] [diff] [review]
netwerk: Add offline mode to DNS service and STS.  Add RESOLVE_OFFLINE flag for nsHostResolver.

When the STS is offline, the socket thread detects that and calls |Reset()|, which detaches the sockets.  If not shutting down, it only detaches non-local sockets.

When the DNS service is offline, it adds a new RESOLVE_OFFLINE flag to calls to |nsHostResolver::ResolveHost()|.  This flag restricts resolution to literals and cached entries.

Should |nsHostResolver::ResolveHost()| still try to resolve when RESOLVE_OFFLINE, but only return loopback records (to allow local hostnames like "localhost")?
Attachment #575552 - Attachment is obsolete: true
Attachment #619220 - Flags: review?(mcmanus)
Comment on attachment 619220 [details] [diff] [review]
netwerk: Add offline mode to DNS service and STS.  Add RESOLVE_OFFLINE flag for nsHostResolver.

Review of attachment 619220 [details] [diff] [review]:
-----------------------------------------------------------------

r=mcmanus with the processpendingevnets() issue addressed. Thanks!

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +1082,5 @@
>  
>      nsresult rv;
>  
> +    if (gIOService->IsOffline() &&
> +        PR_IsNetAddrType(&mNetAddr, PR_IpAddrLoopback) == PR_FALSE)

nit - style is to check !foo instead of foo == false

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +726,5 @@
> +
> +    // Final pass over the event queue. This makes sure that events posted by
> +    // socket detach handlers get processed.
> +    nsIThread *thread = NS_GetCurrentThread();
> +    NS_ProcessPendingEvents(thread);

2 problems here - I think you only want this on the real shutdown, not on the offline case.. and more importantly you now spin the event loop while holding the lock (back down 1 step on the stack) - I was able to deadlock this patch pretty easily because of that. but to fix it I think you just need to only spin it manually on shutdown after the lock is given up, like before.
Attachment #619220 - Flags: review?(mcmanus) → review+
there's an idl change here, so don't forget the sr.
(Assignee)

Comment 29

5 years ago
Created attachment 625811 [details] [diff] [review]
Avoid deadlocks from events.

This only makes the final pass through the event loop when shutting down.

Calling |Reset| in that locked block has a potential deadlock while running the tests:

> ###!!! ASSERTION: Potential deadlock detected:
> Cyclical dependency starts at
> Mutex : nsAStreamCopier.mLock
> Next dependency:
> Mutex : nsSocketTransportService::mLock (currently acquired)
> Cycle completed at
> Mutex : nsAStreamCopier.mLock
> Deadlock may happen for some other execution

To avoid that possibility, this pulls |Reset| out of the locked block.
Attachment #619220 - Attachment is obsolete: true
Attachment #625811 - Flags: superreview?(cbiesinger)
Comment on attachment 625811 [details] [diff] [review]
Avoid deadlocks from events.

sorry for the delay :(

+++ b/netwerk/base/public/nsASocketHandler.h
+    // for server socket, whether only accepts local connections.

whether -> which, I think

+++ b/netwerk/base/src/nsIOService.cpp
+    nsIIOService *topic = static_cast<nsIIOService *>(this);

topic -> subject

+++ b/netwerk/base/src/nsSocketTransportService2.cpp

I don't really understand why you need goingOffline here, can you explain?

+++ b/netwerk/protocol/http/nsHttpChannel.cpp
-        if (offline)
-            mLoadFlags |= LOAD_ONLY_FROM_CACHE;

hrm, so I see why you're doing this, but a better solution would be highly desirable so that you can at least get at the cached page when you have no internet connection, instead of just an error page...
Attachment #625811 - Flags: superreview?(cbiesinger) → superreview-
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #30)

> +++ b/netwerk/protocol/http/nsHttpChannel.cpp
> -        if (offline)
> -            mLoadFlags |= LOAD_ONLY_FROM_CACHE;
> 
> hrm, so I see why you're doing this, but a better solution would be highly
> desirable so that you can at least get at the cached page when you have no
> internet connection, instead of just an error page...

it's not my patch - so maybe I'm missing something - but I think this change just allows you to go to the network (localhost) if you are offline - but you'll still use the cache for everything.

(or does load_only_from_cache give you back expired entries or something you wouldn't normally get?)
(Assignee)

Comment 32

5 years ago
Created attachment 644067 [details] [diff] [review]
Address feedback

Thanks, Christian.

> > +++ b/netwerk/base/public/nsASocketHandler.h
> > +    // for server socket, whether only accepts local connections.
>
> whether -> which, I think

It was supposed to be whether, but I cleaned up the comment a bit to be clearer.  For server sockets, if they are bound to loopback, then they only accept local connections.

> > +++ b/netwerk/base/src/nsSocketTransportService2.cpp
> 
> I don't really understand why you need goingOffline here, can you explain?

This was what I referred to in comment 29.  An assertion had indicated that deadlock was possible when |Reset| was called in the locked block.  While no deadlockable path jumped out at me when I looked, I wanted to play it safe.

I tried to recreate it, but the warning no longer comes up.  This patch puts |Reset| back in the locked block and removes |goingOffline|.

> > +++ b/netwerk/protocol/http/nsHttpChannel.cpp
> > -        if (offline)
> > -            mLoadFlags |= LOAD_ONLY_FROM_CACHE;
> 
> hrm, so I see why you're doing this, but a better solution would be highly 
> desirable so that you can at least get at the cached page when you have no 
> internet connection, instead of just an error page...

As Patrick mentioned in comment 31, it still tries to load from cache if there's an entry for it.  

For example, if I start the browser and load the Google Search page in a tab, close it, go offline, and try to load Google Search in a new tab, it loads from the cache.  Same thing if I start the browser, immediately go offline, and then try to load a URL from the about:cache list of cache entries.
Assignee: nobody → unusualtears
Attachment #625811 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #644067 - Flags: superreview?(cbiesinger)
(In reply to Adam [:hobophobe] from comment #32)
> As Patrick mentioned in comment 31, it still tries to load from cache if
> there's an entry for it.  
> 
> For example, if I start the browser and load the Google Search page in a
> tab, close it, go offline, and try to load Google Search in a new tab, it
> loads from the cache.  Same thing if I start the browser, immediately go
> offline, and then try to load a URL from the about:cache list of cache
> entries.

Even if the page is expired?
Attachment #644067 - Flags: superreview?(cbiesinger) → superreview+
(In reply to Patrick McManus [:mcmanus] from comment #31)
> (or does load_only_from_cache give you back expired entries or something you
> wouldn't normally get?)

Correct, it does.

Maybe that change is ok...
(Assignee)

Comment 35

5 years ago
Created attachment 655837 [details] [diff] [review]
Unbitrot and prepare for checkin

FWIW I tried the 'expires' test on http://www.procata.com/cachetest/ using the patch.  

STR:
1. Go to http://www.procata.com/cachetest/
2. Click on the Expires "Perform Test" link
3. Click on the "Click here to go to step 2" link
4. Go Offline
5. Click on the "Click here to go to step 3" link (loads the same page as [2], but to pass it must not load the cached version).
6. Hover the "Click here to complete the test" and note it is "fail.html"

In [6], if we weren't loading an expired entry, we would get the Offline error page.  If we were online, it would replace the page with one where the link is to "pass.html"

Unless I'm misunderstanding the test, we're still getting expired entries (either without |LOAD_ONLY_FROM_CACHE|, or it's set later).

I'm also seeing warnings about potential deadlock again.  Is this just a conservative warning, or something to be addressed?

Output:

> ###!!! ASSERTION: Potential deadlock detected:
> Cyclical dependency starts at
> ReentrantMonitor : nsHttpConnectionMgr.mReentrantMonitor
> Next dependency:
> Mutex : nsSocketTransportService::mLock (currently acquired)
> Cycle completed at
> ReentrantMonitor : nsHttpConnectionMgr.mReentrantMonitor
> Deadlock may happen for some other execution

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=7d24d71d64b2
Attachment #644067 - Attachment is obsolete: true
(Assignee)

Comment 36

5 years ago
Created attachment 656680 [details] [diff] [review]
Fix broken tests, move ensureSocketThreadTargetIfOnline to ensureSocketThreadTarget

The |test_error_codes.js| test failure was getting |NS_ERROR_NOT_INITIALIZED| from |nsHTTPConnectionMgr::PostEvent|; |EnsureSocketThreadTargetIfOnline| becomes |EnsureSocketThreadTarget| and the test gets updated.

The other tests broke because localhost is now reachable in offline mode.  The fix for all three is disabling the proxy ("network.proxy.type" to |0|) when those tests are offline.  They would pass when run in isolation, either because the proxy itself hadn't been resolved before or maybe there's some browser-side caching of proxy endpoints?

The potential deadlock situation appears to be the cause for at least one crash [1].  That crash is related to the test for bug 336682, which is offline-mode related, though the test itself passed.  This patch will call |Reset| outside of the locked block.

1. https://tbpl.mozilla.org/php/getParsedLog.php?id=14768084&tree=Try 

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=a1a4ebe12100
Attachment #655837 - Attachment is obsolete: true
(Assignee)

Comment 37

5 years ago
Created attachment 656706 [details] [diff] [review]
Correct order of nsSocketTransportService default constructor arguments

Not sure why this didn't hit before; maybe try didn't have -Werror or had an exception for it before?

Try:
https://tbpl.mozilla.org/?tree=Try&rev=2766d56f0ff9
Attachment #656680 - Attachment is obsolete: true
(Assignee)

Comment 38

5 years ago
Doing a full try push this time, to try to sniff out any remaining problems before I ask for review.

https://tbpl.mozilla.org/?tree=Try&rev=40afcaef2976
(Assignee)

Comment 39

5 years ago
Created attachment 657902 [details] [diff] [review]
Unbitrot

The last patch required slight fuzzing to apply (from the |nsAutoCString| change).
Attachment #656706 - Attachment is obsolete: true
Attachment #657902 - Flags: review?(mcmanus)
> 
> I'm also seeing warnings about potential deadlock again.  Is this just a
> conservative warning, or something to be addressed?
> 

That's quite real.. I think the reset() logic should take care of it - are you confident this is fixed?

> Output:
> 
> > ###!!! ASSERTION: Potential deadlock detected:
> > Cyclical dependency starts at
> > ReentrantMonitor : nsHttpConnectionMgr.mReentrantMonitor
> > Next dependency:
> > Mutex : nsSocketTransportService::mLock (currently acquired)
> > Cycle completed at
> > ReentrantMonitor : nsHttpConnectionMgr.mReentrantMonitor
> > Deadlock may happen for some other execution
> 
> 

(In reply to Adam [:hobophobe] from comment #35)
>
> 6. Hover the "Click here to complete the test" and note it is "fail.html"
> 
> In [6], if we weren't loading an expired entry, we would get the Offline
> error page.  If we were online, it would replace the page with one where the
> link is to "pass.html"
> 
> Unless I'm misunderstanding the test, we're still getting expired entries
> (either without |LOAD_ONLY_FROM_CACHE|, or it's set later).
> 

good test. did you get it resolved so it passes?
Comment on attachment 657902 [details] [diff] [review]
Unbitrot

Review of attachment 657902 [details] [diff] [review]:
-----------------------------------------------------------------

The net effect of this is going to mean the socket thread stays constant even when offline/online events occur. I think that's a really good thing and it will eliminate a few classes of bugs.

r+ contingent on these small items and answers to comment 40.

::: browser/base/content/test/browser_bug435325.js
@@ +40,5 @@
>    finish();
>  }
>  
>  registerCleanupFunction(function() {
> +  Services.prefs.setIntPref("network.proxy.type", 2);

you need at least push/pop. Please comment that this is necessary because the mochitest proxy runs n localhost and localhost is now reachable even when offline.

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +684,5 @@
> +void
> +nsSocketTransportService::Reset(bool aGuardLocals)
> +{
> +    // detach any sockets
> +    PRInt32 i;

you've reintroduced PRint types.. need to scrub for those.

@@ +686,5 @@
> +{
> +    // detach any sockets
> +    PRInt32 i;
> +    bool isGuarded;
> +    for (i=mActiveCount-1; i>=0; --i) {

I know its copy and paste, but fix it up to
for (i = mActiveCouint -1; i >= 0; --i)

@@ +693,5 @@
> +            mActiveList[i].mHandler->IsLocal(&isGuarded);
> +        if (!isGuarded)
> +            DetachSocket(mActiveList, &mActiveList[i]);
> +    }
> +    for (i=mIdleCount-1; i>=0; --i) {

again, fix the spacing

::: toolkit/components/places/tests/browser/browser_bug680727.js
@@ +17,5 @@
>    gBrowser.selectedTab = gBrowser.addTab();
>  
> +  // Bypass proxy temporarily, (local is available in offline, see bug 87717)
> +  Services.prefs.setIntPref("network.proxy.type", 0);
> +

same comments from bug435325

::: toolkit/mozapps/extensions/test/xpinstall/browser_offline.js
@@ +16,5 @@
>  }
>  
>  function download_progress(addon, value, maxValue) {
>    try {
> +    Services.prefs.setIntPref("network.proxy.type", 0);

same comments from bug 435325
Attachment #657902 - Flags: review?(mcmanus)
(Assignee)

Comment 42

5 years ago
Created attachment 658606 [details] [diff] [review]
Address feedback

(In reply to Patrick McManus [:mcmanus] from comment #40)
> > 
> > I'm also seeing warnings about potential deadlock again.  Is this just a
> > conservative warning, or something to be addressed?
> > 
> 
> That's quite real.. I think the reset() logic should take care of it - are
> you confident this is fixed?

Yes.  The fact that |Reset| always happens outside of that lock means it can't cause deadlock.

> (In reply to Adam [:hobophobe] from comment #35)
> >
> > 6. Hover the "Click here to complete the test" and note it is "fail.html"
> > 
> > In [6], if we weren't loading an expired entry, we would get the Offline
> > error page.  If we were online, it would replace the page with one where the
> > link is to "pass.html"
> > 
> > Unless I'm misunderstanding the test, we're still getting expired entries
> > (either without |LOAD_ONLY_FROM_CACHE|, or it's set later).
> > 
> 
> good test. did you get it resolved so it passes?

That test was to check on Christian's concern in comment 33 and comment 34, that before, when offline, we would still get expired pages, and that it might not be happening with this change.

The test gives us a page that should immediately expire.  Failure means we did not grab a fresh copy, passing means we did.

My impression is that we're accepting that offline mode may violate cache expiration, because it can't fetch a fresh copy; that is, that the failure is the Right Thing.  When online, the test passes.

(In reply to Patrick McManus [:mcmanus] from comment #41)
> ::: browser/base/content/test/browser_bug435325.js
> @@ +40,5 @@
> >    finish();
> >  }
> >  
> >  registerCleanupFunction(function() {
> > +  Services.prefs.setIntPref("network.proxy.type", 2);
> 
> you need at least push/pop. Please comment that this is necessary because
> the mochitest proxy runs n localhost and localhost is now reachable even
> when offline.

Added the comments, but wasn't clear if the "push/pop" referred to something else?
Attachment #657902 - Attachment is obsolete: true
Attachment #658606 - Flags: review?(mcmanus)
> 
> (In reply to Patrick McManus [:mcmanus] from comment #41)
> > ::: browser/base/content/test/browser_bug435325.js
> > @@ +40,5 @@
> > >    finish();
> > >  }
> > >  
> > >  registerCleanupFunction(function() {
> > > +  Services.prefs.setIntPref("network.proxy.type", 2);
> > 
> > you need at least push/pop. Please comment that this is necessary because
> > the mochitest proxy runs n localhost and localhost is now reachable even
> > when offline.
> 
> Added the comments, but wasn't clear if the "push/pop" referred to something
> else?

by push/pop I mean you need to save the original state of the pref and restore that rather than setting it explicitly to "2" when we're done, in case that's not what is used in the future.
(Assignee)

Comment 44

5 years ago
Created attachment 658634 [details] [diff] [review]
Save and restore the network proxy preference value in tests for robustness
Attachment #658606 - Attachment is obsolete: true
Attachment #658606 - Flags: review?(mcmanus)
Attachment #658634 - Flags: review?(mcmanus)
Attachment #658634 - Flags: review?(mcmanus) → review+
(Assignee)

Updated

5 years ago
Attachment #658634 - Flags: superreview?(cbiesinger)
> My impression is that we're accepting that offline mode may violate cache expiration, because > it can't fetch a fresh copy; that is, that the failure is the Right Thing.  When online, the > test passes

From the user's point of view, I wouldn't call it "the right thing". We're not showing her a page even though we have the data.
Comment on attachment 658634 [details] [diff] [review]
Save and restore the network proxy preference value in tests for robustness

Review of attachment 658634 [details] [diff] [review]:
-----------------------------------------------------------------

This patch will still not allow you to connect to "localhost" when offline, right? Just to ::1/127.0.0.1

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ -409,5 @@
>  
> -    // are we offline?
> -    bool offline = gIOService->IsOffline();
> -    if (offline)
> -        mLoadFlags |= LOAD_ONLY_FROM_CACHE;

anyway, I'm ok with this removal. just saying it's not clear that it's the right thing.
Attachment #658634 - Flags: superreview?(cbiesinger) → superreview+
(Assignee)

Comment 47

5 years ago
Created attachment 660621 [details] [diff] [review]
Prepare for checkin

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=2b84bacf7276

> From the user's point of view, I wouldn't call it "the right 
> thing". We're not showing her a page even though we have the
> data. 

and

> anyway, I'm ok with this removal. just saying it's not clear that 
> it's the right thing.

We _are_ showing her the page.  That's why the test points to "fail.html", because we loaded/displayed an expired, cached page.  The removal does not block that behavior, only allows local connections prior to that.

> This patch will still not allow you to connect to "localhost"
> when offline, right? Just to ::1/127.0.0.1 

It could connect to "localhost" through a local proxy (why the tests were failing before), but otherwise it won't.  If you connected to "localhost" recently before going offline, it will load a cached copy.
Attachment #658634 - Attachment is obsolete: true
(Assignee)

Comment 48

5 years ago
Created attachment 661458 [details] [diff] [review]
Initialize STS when offline

Sorry for the review churn here.  Previous patch wasn't passing try for Android, which I had thought was due to an intermittent problem but proved to be patch's fault.

Change here is in STS::Init, allowing initialization when offline.

Greenish try is:
https://tbpl.mozilla.org/?tree=Try&rev=86c7cc62bde9
Attachment #660621 - Attachment is obsolete: true
Attachment #661458 - Flags: review?(mcmanus)
Attachment #661458 - Flags: review?(mcmanus) → review+
i would think you can carry the sr forward
(Assignee)

Comment 50

5 years ago
Created attachment 662281 [details] [diff] [review]
Prepare for checkin

Thanks, Patrick!
Attachment #661458 - Attachment is obsolete: true
(Assignee)

Comment 51

5 years ago
Green try at:
https://tbpl.mozilla.org/?tree=Try&rev=9147e7e13b02
Keywords: helpwanted → checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c122628565e5
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c122628565e5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Blocks: 795816

Updated

5 years ago
Blocks: 806205
Depends on: 814430
Depends on: 828630

Updated

4 years ago
Depends on: 829587
If I switch Firefox to offline mode in current nightly and type in "http://localhost/", it tells me Firefox is offline and can't browse the web. Surely I should expect it to work?

Gerv
(Assignee)

Comment 55

4 years ago
(In reply to Gervase Markham [:gerv] from comment #54)
> If I switch Firefox to offline mode in current nightly and type in
> "http://localhost/", it tells me Firefox is offline and can't browse the
> web. Surely I should expect it to work?

The localhost name is just a name.  It follows the normal process to resolve to an address.  So it only works if localhost is in the DNS cache (ie, you recently connected to it while online), because name resolution of non-cached names doesn't occur in offline mode.

The patch itself only enables connection to loopback interfaces.  It doesn't modify the DNS resolver to try to resolve unknown names in offline mode.

The browser doesn't have any say over the resolution process once it asks for a record.  That is, we can't restrict a lookup to only the hosts file (where localhost and other locally defined names are configured).  An attempted lookup that doesn't resolve in the hosts file would try to hit the network.

If that's deemed acceptable, then a subsequent patch could simply filter resolution requests in offline mode.  That is, only pass them on if they are loopback.  But it would mean that we would cause network traffic in "offline" mode.

Comment 56

4 years ago
For me, all this patching was just a bad idea from modem era to save couple of bytes.

When I want to disable internet activity, I click on the network connection icon in my OS GUI (any Linux or Windows) and use disconnect option. And I use browser offline mode to avoid ANY browser network activity with web sites, local proxy, DNS, external/local mail server (in SeaMonkey and Thunderbird) etc. I just need to view a cached web page or to stop looped JS refreshes of pages opened.

It was browser offline mode, not system one. And now we have "localhost" requests depending on browser DNS cache.

Is there any workaround to close all sockets or just to prevent browser from using them?
(Assignee)

Comment 57

4 years ago
Not sure how strict your use case is.  A local proxy might fit, but not if you're trying to prevent even socket creation; for the latter something like SELinux or systrace might work, but harder to set up and manage.

Comment 58

4 years ago
Well... People who wanted to control the OS NETWORK ACCESS per program got this firewall feature in the browser core. People who wanted to control the BROWSER to use or not to use networking got regression with the broken behavior.
It looks like RESOLVE_OFFLINE is unused now. In fact it looks like it was never used in the patch landed in this bug. How does this work, or is that a mistake?
Flags: needinfo?(unusualtears)
Not sure if hobophobe is active any more, so also tagging mcmanus with needinfo as reviewer.
Flags: needinfo?(mcmanus)
(Assignee)

Comment 61

2 years ago
Unless I'm missing something, I guess this is a different-names-broke-search problem? It's called |RES_OFFLINE| when it's actually checked/used: http://hg.mozilla.org/mozilla-central/file/9605da94e75d/netwerk/dns/nsHostResolver.cpp#l826 

The motivation for the different namings was due to the fact that flags in the IDL (|nsIDNSService.idl|) get named as |RESOLVE_FOO| while flags in the |nsHostResolver.h| name as |RES_FOO|.
Flags: needinfo?(unusualtears)
Oh man, that's awful. Thanks for the prompt response.
Flags: needinfo?(mcmanus)
You need to log in before you can comment on or make changes to this bug.