Closed Bug 1239955 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
blocking-b2g 2.5+
Tracking Status
firefox47 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: junior, Assigned: kershaw)

References

Details

(Whiteboard: [ft:conndevices])

Attachments

(2 files, 1 obsolete file)

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.
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.
Flags: needinfo?(daniel)
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.
Flags: needinfo?(daniel)
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
Flags: needinfo?(daniel)
(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?
Flags: needinfo?(daniel)
(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?
Flags: needinfo?(daniel)
(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.
Flags: needinfo?(daniel)
Blocks: 1236842
Hi Daniel,

Could you please take a look at this patch?

Thanks.
Attachment #8709886 - Flags: review?(daniel)
(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.
Flags: needinfo?(mcmanus)
have you thought about e10s issues here?
Flags: needinfo?(mcmanus)
(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.
Attachment #8709886 - Flags: review?(daniel) → review?(mcmanus)
Duplicate of this bug: 1236842
Blocks: TV_P1
Hi Patrick, 
Could you kindly review the patch? Thanks!
Flags: needinfo?(mcmanus)
Whiteboard: [ft:conndevices]
Attachment #8709886 - Flags: review?(mcmanus)
Attachment #8709886 - Flags: review?(daniel)
Attachment #8709886 - Flags: feedback+
Hi Daniel, 
Could you kindly review the patch? Thanks!
Assignee: nobody → kechang
Flags: needinfo?(mcmanus) → needinfo?(daniel)
blocking-b2g: --- → 2.5+
Blocks: 1236852
Blocks: 1244016
if any progress here?
Comment on attachment 8709886 [details] [diff] [review]
Let DNSService rely on IOService::Offline

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

LGTM!
Attachment #8709886 - Flags: review?(daniel) → review+
Flags: needinfo?(daniel)
Carry reviewer's name.

Thanks for reviewing this.
Attachment #8709886 - Attachment is obsolete: true
Attachment #8720664 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0712c88206ab
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
This is rebased for B2G 2.5.
Attachment #8722425 - Flags: review+
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
Attachment #8722425 - Flags: approval‑mozilla‑b2g44?
(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.
Flags: needinfo?(jocheng)
Comment on attachment 8722425 [details] [diff] [review]
Let DNSService rely on IOService::Offline, r=bagder (for B2G 2.5)

Approve for TV 2.5
Flags: needinfo?(jocheng)
Attachment #8722425 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
Keywords: checkin-needed
@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.
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.