superreview requested: [Bug 87717] Conn: Offline: localhost/127.0.0.1 not accessible : [Attachment 625811] Avoid deadlocks from events.

[bugzill...@mozilla.org]

Adam [:hobophobe] <unusua...@gmail.com> has asked Christian :Biesinger
(don't email me, ping me on IRC) <cbies...@gmail.com> for superreview:
Bug 87717: Conn: Offline: localhost/127.0.0.1 not accessible
https://bugzilla.mozilla.org/show_bug.cgi?id=87717

Attachment 625811: Avoid deadlocks from events.
https://bugzilla.mozilla.org/attachment.cgi?id=625811&action=edit


------- Additional Comments from Adam [:hobophobe] <unusua...@gmail.com>
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.

[bugzill...@mozilla.org]

Christian :Biesinger (don't email me, ping me on IRC) <cbies...@gmail.com>

has not granted Adam [:hobophobe] <unusua...@gmail.com>'s request for

superreview:
Bug 87717: Conn: Offline: localhost/127.0.0.1 not accessible
https://bugzilla.mozilla.org/show_bug.cgi?id=87717

Attachment 625811: Avoid deadlocks from events.
https://bugzilla.mozilla.org/attachment.cgi?id=625811&action=edit

------- Additional Comments from Christian :Biesinger (don't email me, ping me
on IRC) <cbies...@gmail.com>
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...

[bugzill...@mozilla.org]

Adam [:hobophobe] <unusua...@gmail.com> has asked Christian :Biesinger
(don't email me, ping me on IRC) <cbies...@gmail.com> for superreview:

Bug 87717: Conn: Offline: localhost/127.0.0.1 not accessible
https://bugzilla.mozilla.org/show_bug.cgi?id=87717

Attachment 644067: Address feedback
https://bugzilla.mozilla.org/attachment.cgi?id=644067&action=edit

------- Additional Comments from Adam [:hobophobe] <unusua...@gmail.com>

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.

[bugzill...@mozilla.org]

Christian :Biesinger (don't email me, ping me on IRC) <cbies...@gmail.com>

has granted Adam [:hobophobe] <unusua...@gmail.com>'s request for

[bugzill...@mozilla.org]

Adam [:hobophobe] <unusua...@gmail.com> has asked Christian :Biesinger
(don't email me, ping me on IRC) <cbies...@gmail.com> for superreview:

Bug 87717: Conn: Offline: localhost/127.0.0.1 not accessible
https://bugzilla.mozilla.org/show_bug.cgi?id=87717

Attachment 658634: Save and restore the network proxy preference value in tests
for robustness
https://bugzilla.mozilla.org/attachment.cgi?id=658634&action=edit

[bugzill...@mozilla.org]

Christian :Biesinger (don't email me, ping me on IRC) <cbies...@gmail.com>

has granted Adam [:hobophobe] <unusua...@gmail.com>'s request for

superreview:
Bug 87717: Conn: Offline: localhost/127.0.0.1 not accessible
https://bugzilla.mozilla.org/show_bug.cgi?id=87717

Attachment 658634: Save and restore the network proxy preference value in tests
for robustness
https://bugzilla.mozilla.org/attachment.cgi?id=658634&action=edit

------- Additional Comments from Christian :Biesinger (don't email me, ping me
on IRC) <cbies...@gmail.com>
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.