1239955 - Should pop NS_ERROR_OFFLINE if a non-gonk agent is without connectivity

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Junior [inactive]]

We have tested in Linux and OS X

STR:
1. Disconnect wifi and ethernet
2. Browse a non-cached website

Expected behaviour:
Indicate user this agent is without connectivity.

Actual behaviour:
"Server not found" is shown.

Comment 1

[Junior [inactive]]

Hello Daniel,

In Linux, we found that both |IOService::SetConnectiviy| and |IOService::SetOffline| are not triggered when we disconnect the internet. Therefore, DNSService is not able to tell the connectivity.

Moreover, DNSService seems to depend on offline status instead of (offline || !connectivity).

It's a little weird to has a non-local behaviour without connectivity.
Is this behaviour by design?

Thanks.

Comment 2

[Daniel Stenberg [:bagder]]

This is by design, but there's more to it. Let me tell you my view of it all. First: "connectivity" and "offline" are not the same thing (any more). The previous stricter association between them caused a lot of head aches (mostly due to the trickiness in perfectly detecting connectivity with no false positives).

Connectivity is what we programmatically have detected and believe the status is on the "link" right now.

Offline is the status we like the browser to consider it to be in. You can for example set it to offline manually, and it is then set offline completely independently of what the connectivity is.

So, 'offline' is the state that would restrict Firefox from doing things that require that it is online. It could then actually consider itself online while the connectivity goes to false.

There's a pref called 'network.offline-mirrors-connectivity' (false by default) that if enabled sets Firefox's offline mode in sync with the connectivity.

Comment 3

[Junior [inactive]]

Thanks for your quick response :)
Now I know the difference between "offline" and "connectivity"

Two follow-up questions:
1. If connectivity == false, necko would return NS_ERROR_UNKNOWN_HOST to docshell and redirect the error to gaia in b2g case. Therefore, gaia can not tell if it's a real dns error or a connectivity error. Is this behaviour also by design?

2. I haven't test "network.offline-mirrors-connectivity" pref since |IOService::SetConnectiviy| seems not called in Linux. IIRC even if the pref is on, the DNSService still doesn't rely on connectivity since the only chance to change |mOffline| is here.
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#1029

Comment 4

[Daniel Stenberg [:bagder]]

(In reply to Junior [:junior] from comment #3)

> 1. If connectivity == false, necko would return NS_ERROR_UNKNOWN_HOST to
> docshell and redirect the error to gaia in b2g case. Therefore, gaia can not
> tell if it's a real dns error or a connectivity error. Is this behaviour
> also by design?

When would necko return this you mean? I *believe* the intent is that NS_ERROR_OFFLINE should be returned when Firefox is offline, and when we're not offline we're doing the normal procedures and then I would guess name resolving without a network connection typically fails with an NS_ERROR_UNKNOWN_HOST unless you resolve a name that's already cached, in /etc/hosts or similar.

If we're only using the offline status to stop us from using the network, then I think it makes sense that if we're online we try to resolve the host using the normal means.

> 2. I haven't test "network.offline-mirrors-connectivity" pref since
> |IOService::SetConnectiviy| seems not called in Linux.

SetConnectivityInternal() should be called on Linux afaics.

> IIRC even if the pref
> is on, the DNSService still doesn't rely on connectivity since the only
> chance to change |mOffline| is here.
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.
> cpp#1029

I agree! It seems the "linking" is mostly for nsIOService->GetOffline() but unfortunately the DNSService doesn't use that method, nsIOService sets the offline status explicitly for it with mDNSService->SetOffline(). It looks like a potential bug to me. You agree?

Comment 5

[Junior [inactive]]

(In reply to Daniel Stenberg [:bagder] from comment #4)

> When would necko return this you mean? 

Finally docshell would generate a link like about:neterror?e=dnsNotFound
according to the error from Necko (or other component).

> 
> > 2. I haven't test "network.offline-mirrors-connectivity" pref since
> > |IOService::SetConnectiviy| seems not called in Linux.
> 
> SetConnectivityInternal() should be called on Linux afaics.
> 
I see the logic here, but the gdb doesn't stop. Let me check it carefully later.
https://dxr.mozilla.org/mozilla-central/source/netwerk/system/linux/nsNotifyAddrListener_Linux.cpp#532

> > IIRC even if the pref
> > is on, the DNSService still doesn't rely on connectivity since the only
> > chance to change |mOffline| is here.
> > https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.
> > cpp#1029
> 
> I agree! It seems the "linking" is mostly for nsIOService->GetOffline() but
> unfortunately the DNSService doesn't use that method, nsIOService sets the
> offline status explicitly for it with mDNSService->SetOffline(). It looks
> like a potential bug to me. You agree?

Yes I agree.


Let's say: if gaia wants to tell if it's a real dns error or a connectivity error,
we should 
1. enable the pref "network.offline-mirrors-connectivity"
2. Let mOffline of DNSService rely on IOService::GetConnectivity.

How about this idea?

Comment 6

[Daniel Stenberg [:bagder]]

(In reply to Junior [:junior] from comment #5)

> Let's say: if gaia wants to tell if it's a real dns error or a connectivity
> error,
> we should 
> 1. enable the pref "network.offline-mirrors-connectivity"
> 2. Let mOffline of DNSService rely on IOService::GetConnectivity.

For this (2), it would be enough if DNSService relied on IOService::Offline instead of having its own offline mode storage, as then the (1) pref would do its work properly and it would remain working like now when the pref is false.

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: Let DNSService rely on IOService::Offline

Hi Daniel,

Could you please take a look at this patch?

Thanks.

Comment 8

[Daniel Stenberg [:bagder]]

(In reply to Kershaw Chang [:kershaw] from comment #7)

> Could you please take a look at this patch?

I think it looks fine, and it even simplifies the code a bit I think by centralizing the offline status more.

But I'd like to ask :mcmanus for his view on the approach, plus either his review or hand-off to me for reviewing as I'm not a network peer.

Comment 9

[Patrick McManus [:mcmanus]]

have you thought about e10s issues here?

Comment 10

[Kershaw Chang [:kershaw]]

(In reply to Patrick McManus [:mcmanus] from comment #9)
> have you thought about e10s issues here?

Do you mean if the DNSService can work in content process?

I also modified ChildDNSService to get the offline status from IOService, so I think this should be no problem in content process.

Comment 12

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d12dab32050

Comment 13

[Josh Cheng [:josh]]

Hi Patrick, 
Could you kindly review the patch? Thanks!

Comment 14

[Josh Cheng [:josh]]

Hi Daniel, 
Could you kindly review the patch? Thanks!

Comment 15

[Mike Lien[:mlien]]

if any progress here?

Comment 16

[Daniel Stenberg [:bagder]]

Comment on attachment 8709886 [details] [diff] [review]
Let DNSService rely on IOService::Offline

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

LGTM!

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: Bug 1239955-Let DNSService rely on IOService Offline, r=bagder

Carry reviewer's name.

Thanks for reviewing this.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0712c88206ab

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0712c88206ab

Comment 20

[Kershaw Chang [:kershaw]]

Attachment: Let DNSService rely on IOService::Offline, r=bagder (for B2G 2.5)

This is rebased for B2G 2.5.

Comment 21

[Kershaw Chang [:kershaw]]

Comment on attachment 8722425 [details] [diff] [review]
Let DNSService rely on IOService::Offline, r=bagder (for B2G 2.5)

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Offline status of DNSService may be inconsistent with IOService.
Testing completed: Local test.
Risk to taking this patch (and alternatives if risky): Low, since it's a small change.
String or UUID changes made by this patch: n/a

Comment 22

[Kershaw Chang [:kershaw]]

(In reply to Kershaw Chang [:kershaw] from comment #21)
> Comment on attachment 8722425 [details] [diff] [review]
> Let DNSService rely on IOService::Offline, r=bagder (for B2G 2.5)
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): n/a
> User impact if declined: Offline status of DNSService may be inconsistent
> with IOService.
> Testing completed: Local test.
> Risk to taking this patch (and alternatives if risky): Low, since it's a
> small change.
> String or UUID changes made by this patch: n/a

Josh, could you help to approve this? Thanks.

Comment 23

[Josh Cheng [:josh]]

Comment on attachment 8722425 [details] [diff] [review]
Let DNSService rely on IOService::Offline, r=bagder (for B2G 2.5)

Approve for TV 2.5

Comment 24

[Kershaw Chang [:kershaw]]

@Tomcat, could you please help to commit this patch?(In reply to Josh Cheng [:josh] from comment #23)
> Comment on attachment 8722425 [details] [diff] [review]
> Let DNSService rely on IOService::Offline, r=bagder (for B2G 2.5)
> 
> Approve for TV 2.5

@Tomcat, could you help to commit this patch? Thanks.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8a1846fcd859