firefox should be smarter about offline access

[Kevin Hunter]

Binary package hint: firefox

The term 'offline' is just a euphemism for "net-not-accessible." As such, even when the net is not accessible, certain items *are* accessible. For instance, http://127.0.0.1 is *always* accessible.

Further, if the user has special entries in their host file, or has a local copy of DNS for certain things, then these *could* be resolved. I propose that Firefox should be smarter about offline access. It could try for "one hop" to find resolve names.

For instance, if I have this in my hosts file

127.0.0.1 localhost
127.0.0.1 somelocalproject
127.0.0.1 anotherlocalproject

Then all three of

http://localhost/
http://somelocalproject/
http://anotherlocalproject/

should be accessible.

[Benc-meer (benc-meer)]

I can think of arguments for this both ways. Can someone post to the newsgroup
and moderate this?

[basic (basic)]

127.0.0.1 shouldn't work in offline mode, but localhost might.

[Malx (malx)]

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.

[basic (basic)]

If they are the same, then this bug is invalid. 127.0.0.1 is used for loopback

[Malx (malx)]

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" :)

[Jh-junetz (jh-junetz)]

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.

[Benc-meer (benc-meer)]

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.

[Asa (asa)]

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)

[Benc-meer (benc-meer)]

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.

[Mozbugz-dougt (mozbugz-dougt)]

moving neeti's futured bugs for triaging.

[Mozilla-com (mozilla-com)]

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.

[Benc-meer (benc-meer)]

-> 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.

[Benc-meer (benc-meer)]

fixed dependency.

[Ajimix (ajimix)]

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?

[Mozbugz-eatmorespuds (mozbugz-eatmorespuds)]

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".

[Kevin Hunter (hunteke)]

Binary package hint: firefox

The term 'offline' is just a euphemism for "net-not-accessible." As such, even when the net is not accessible, certain items *are* accessible. For instance, http://127.0.0.1 is *always* accessible.

Further, if the user has special entries in their host file, or has a local copy of DNS for certain things, then these *could* be resolved. I propose that Firefox should be smarter about offline access. It could try for "one hop" to find resolve names.

For instance, if I have this in my hosts file

127.0.0.1 localhost
127.0.0.1 somelocalproject
127.0.0.1 anotherlocalproject

Then all three of

http://localhost/
http://somelocalproject/
http://anotherlocalproject/

should be accessible.

[Craig Silk (csilk)]

I totally agree with you. This is a genuine useability issue in Firefox. Have you considered sending a feature request to Mozilla, Making a mailing list post, or adding this to their bugzilla?

[Kevin Hunter (hunteke)]

Yeah, I've considered those, but I opted not to. I don't know where the customizations come in for the distro-specific stuff, nor am I a developer; I'm just a semi-knowledgeable end-user. (I know enough to get myself in trouble!)

[Alexander Sack (asac)]

this is a wishlist bug that needs to be done upstream. search bugzilla.mozilla.org for similar bugs ... if none exist, file a new bug and post your bug id here. Thanks!

[Kevin Hunter (hunteke)]

Okay, don't know how to attach/follow an upstream report, but I believe this bug is the one that I described here.

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

[Kevin Hunter (hunteke)]

Update: 436815 corrected me. The bug to watch is actually

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

Unfortunately, it appears to have been open since 2001. Coming up on 8 years ...

[Pascal-sartoretti-elca (pascal-sartoretti-elca)]

*** Bug 531024 has been marked as a duplicate of this bug. ***

[Christian Biesinger (cbiesinger)]

*** Bug 432434 has been marked as a duplicate of this bug. ***

[Martin von Gagern (gagern)]

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.

[Adam Dane (unusualtears)]

Created attachment 573692
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

[Adam Dane (unusualtears)]

Created attachment 575552
netwerk: single check for observer service before notifying

[Adam Dane (unusualtears)]

Created attachment 575555
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.

[Adam Dane (unusualtears)]

Created attachment 575570
NSPR: Add a pair of tests (one IPv4, one IPv6 with IPv4 mapped address)

[Matti-mversen (matti-mversen)]

Adam: You have to request reviews from someone !

[Josh Matthews (joshmatthews)]

Comment on attachment 575552
netwerk: single check for observer service before notifying

Jason chosen by random selection.

[Patrick McManus (mcmanus-ducksong)]

Comment on attachment 575552
netwerk: single check for observer service before notifying

Review of attachment 575552:
-----------------------------------------------------------------

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.

[Adam Dane (unusualtears)]

Created attachment 619220
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")?

[Patrick McManus (mcmanus-ducksong)]

Comment on attachment 619220
netwerk: Add offline mode to DNS service and STS. Add RESOLVE_OFFLINE flag for nsHostResolver.

Review of attachment 619220:
-----------------------------------------------------------------

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.

[Patrick McManus (mcmanus-ducksong)]

there's an idl change here, so don't forget the sr.

[Adam Dane (unusualtears)]

Created attachment 625811
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.

[Christian Biesinger (cbiesinger)]

Comment on attachment 625811
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...

[Patrick McManus (mcmanus-ducksong)]

(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?)

[Adam Dane (unusualtears)]

Created attachment 644067
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.

[Christian Biesinger (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?

[Christian Biesinger (cbiesinger)]

(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...

[Adam Dane (unusualtears)]

Created attachment 655837
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

[Adam Dane (unusualtears)]

Created attachment 656680
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

[Adam Dane (unusualtears)]

Created attachment 656706
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

[Adam Dane (unusualtears)]

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

[Adam Dane (unusualtears)]

Created attachment 657902
Unbitrot

The last patch required slight fuzzing to apply (from the |nsAutoCString| change).

[Patrick McManus (mcmanus-ducksong)]

>
> 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?

[Patrick McManus (mcmanus-ducksong)]

Comment on attachment 657902
Unbitrot

Review of attachment 657902:
-----------------------------------------------------------------

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

[Adam Dane (unusualtears)]

Created attachment 658606
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?

[Patrick McManus (mcmanus-ducksong)]

>
> (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.

[Adam Dane (unusualtears)]

Created attachment 658634
Save and restore the network proxy preference value in tests for robustness

[Christian Biesinger (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.

[Christian Biesinger (cbiesinger)]

Comment on attachment 658634
Save and restore the network proxy preference value in tests for robustness

Review of attachment 658634:
-----------------------------------------------------------------

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.

[Adam Dane (unusualtears)]

Created attachment 660621
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.

[Adam Dane (unusualtears)]

Created attachment 661458
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

[Patrick McManus (mcmanus-ducksong)]

i would think you can carry the sr forward

[Adam Dane (unusualtears)]

Created attachment 662281
Prepare for checkin

Thanks, Patrick!

[Adam Dane (unusualtears)]

Green try at:
https://tbpl.mozilla.org/?tree=Try&rev=9147e7e13b02

[Jaws (jaws)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c122628565e5

[Graememcc-firefox (graememcc-firefox)]

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

[Gervase Markham (gerv-mozilla)]

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

[Adam Dane (unusualtears)]

(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.

[Chalx (chalx)]

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?

[Adam Dane (unusualtears)]

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.

[Chalx (chalx)]

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.