Closed Bug 1132693 Opened 5 years ago Closed 5 years ago

regression: offline detection not working on Android?

Categories

(Core :: Networking, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jduell.mcbugs, Assigned: valentin)

References

Details

Attachments

(2 files)

Valentin just noticed that the test case in bug 729951 comment 0 no longer works.  I've verified that on my own nightly android.

A likely culprit is this line:

  https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#1416

Should be !mManageOfflineStatus?
Daniel, could you look over this patch for a bit?
I noticed recently that network.manage-offline-status=true didn't really work anymore, and I tracked it down to https://hg.mozilla.org/mozilla-central/rev/c886f3cef1a9

The attached patch seems to fix the problem on Windows. It doesn't on Linux, but I think that may be because nsNotifyAddrListener_Linux.cpp::GetIsLinkUp always returns true. We need to fix that too, as it's a major regression of Bug 729951.
Flags: needinfo?(daniel)
I think the patch looks fine and makes sense.

Regarding the detection of offline on Linux desktop, I think we need to add code that scans through the available interfaces at init and when the network changes (much like the windows logic).

But this is the bug about Android, does the fix improve things there?
Flags: needinfo?(daniel)
(In reply to Daniel Stenberg [:bagder] from comment #3)
> But this is the bug about Android, does the fix improve things there?

If Android uses the Linux AddrListener, I doubt it fixes the issue. I'll test it soon with this try build.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=537797cf6951
Comment on attachment 8564151 [details] [diff] [review]
navigator.onLine detection not working on Android

I checked the try build and it seems this fixes the bug for Android as well.
Requesting review while I figure out how to write a test for this (seems tricky)
Attachment #8564151 - Flags: review?(mcmanus)
Attachment #8564151 - Flags: review?(mcmanus) → review+
(In reply to Valentin Gosu [:valentin] from comment #4)

> If Android uses the Linux AddrListener, I doubt it fixes the issue.

It doesn't. The Linux AddrListener is only used by Linux desktop and FxOS, Android has its own version: https://mxr.mozilla.org/mozilla-central/source/netwerk/system/android/nsAndroidNetworkLinkService.cpp
https://hg.mozilla.org/mozilla-central/rev/91c3193203ec
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8564151 [details] [diff] [review]
navigator.onLine detection not working on Android

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

::: netwerk/base/nsIOService.cpp
@@ -1413,5 @@
>      if (mShutdown)
>          return NS_ERROR_NOT_AVAILABLE;
>  
> -    if (mManageOfflineStatus)
> -        return NS_OK;

So are we sure we don't need this line (but with "!mManageOfflineStatus")?  I just saw my browser (can't remember if it was linux or osx--probably osx) give me the "you are in offline mode" page when I tried to browse (and my internet was working fine, so it was wrong).  We should never display that page for any platform (like desktop) that has network.manage-offline-status set to false.

And of course now I can't seem to duplicate it.  But this seems like the most likely culprit.
Flags: needinfo?(valentin.gosu)
I removed that check because the callsites seem to check for it. But you're right, the check should really be done in that function. I'm not sure this is the reason for that bug, though
Attachment #8566323 - Flags: review?(jduell.mcbugs)
Reopening bug to add the check back.
Status: RESOLVED → REOPENED
Flags: needinfo?(valentin.gosu)
Resolution: FIXED → ---
Attachment #8566323 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/62bc66380845
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
I'm not entirely happy with this change:

The CHANGED event is meant to only signal a network change, not an UP/DOWN state change so doing this extra call on a CHANGED event should not be necessary. I'd say this rather looks like there's a missing UP or DOWN event causing this problem, but this change now makes it not becoming a problem.
You need to log in before you can comment on or make changes to this bug.