Send NS_NETWORK_LINK_DATA_CHANGED events on Android

RESOLVED FIXED in mozilla35

Status

()

Core
Networking
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

unspecified
mozilla35
All
Android
Points:
---

Firefox Tracking Flags

(firefox32 wontfix, fennec34+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Bug 939318 added some stuff that allows listeners to respond to a NS_NETWORK_LINK_TOPIC CHANGED event. We need to send this appropriately on Android.
Assignee: nobody → snorp
tracking-fennec: ? → 32+
Status: NEW → ASSIGNED
Created attachment 8445247 [details] [diff] [review]
Send network link change events
Attachment #8445247 - Flags: review?(blassey.bugs)
Comment on attachment 8445247 [details] [diff] [review]
Send network link change events

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

As discussed on IRC, I'm concerned that we'll miss link changes where the type doesn't change, like moving from one WiFi AP to another or switching networks on a dual SIM phone
Attachment #8445247 - Flags: review?(blassey.bugs) → review-
:bagder has some wip that builds a checksum for the windows networking state (current addresses and interfaces, etc..).. you might coordinate

he's on vacation for a bit but I'm thinking of bug 939318
Flags: needinfo?(daniel)
(In reply to Patrick McManus [:mcmanus] from comment #3)
> :bagder has some wip that builds a checksum for the windows networking state
> (current addresses and interfaces, etc..).. you might coordinate
> 
> he's on vacation for a bit but I'm thinking of bug 939318

One thing Brad and I considered was just sending the change event whenever Android tells us there was a change. Would that cause any bad stuff if nothing important did actually change? Presumably it would just cause a few extra pings?
Created attachment 8445347 [details] [diff] [review]
Send network link change events
Attachment #8445247 - Attachment is obsolete: true
Attachment #8445347 - Flags: review?(mcmanus)
Attachment #8445347 - Flags: review?(blassey.bugs)
Attachment #8445347 - Flags: review?(blassey.bugs) → review+
Comment on attachment 8445347 [details] [diff] [review]
Send network link change events

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

I think for the moment this is fine, but a lot of it going to depend on exactly how bagder handles the notifications - so he should consider this and any changes before his other code lands.. I know it will lead to clearing of some pretty important caches, so we want to avoid that if e.g. the wifi signal just blips on and off as it goes in and out of your pocket.
Attachment #8445347 - Flags: review?(mcmanus) → review+
Duplicate of this bug: 1029311
Duplicate of this bug: 1028778
I think it is fine to send the event more often rather than too seldom (if there's ever such a choice), but I added a checksum for the Windows backend in Bug 939318 since its notifying system is very trigger-happy and caused a large amount of events to happen without me seeing any "interesting" info changing. I'm sure that will differ on specific backend system functionality. And again, triggering CHANGED a little too often will "just" hurt some caches and some performance but will be better than missing a network change event that then instead will end up with stalls and annoyed users.
Flags: needinfo?(daniel)

Updated

3 years ago
tracking-fennec: 32+ → ?
status-firefox32: --- → wontfix
tracking-fennec: ? → 34+
This looks like we try to call FlushCache() on a null nsHostResolver. :bagder can you have a look?
Flags: needinfo?(daniel)
Created attachment 8496103 [details] [diff] [review]
Guard against null resolver when flushing DNS cache

Daniel/Patrick, whoever can get to this first :)
Attachment #8496103 - Flags: review?(mcmanus)
Attachment #8496103 - Flags: review?(daniel)
Flags: needinfo?(daniel)
Comment on attachment 8496103 [details] [diff] [review]
Guard against null resolver when flushing DNS cache

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

I'm gonna give a drive-by r+ here, taking Daniel's review because it's late in Europe :)
Looks like mHostResolver can be null if we've previously been told to go offline. So, null checking seems like the right thing to do.
Attachment #8496103 - Flags: review?(daniel) → review+
Comment on attachment 8496103 [details] [diff] [review]
Guard against null resolver when flushing DNS cache

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

steve's review is fine
Attachment #8496103 - Flags: review?(mcmanus)
https://hg.mozilla.org/mozilla-central/rev/ea3b44a4b97a
https://hg.mozilla.org/mozilla-central/rev/473717699910
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

Updated

3 years ago
Duplicate of this bug: 1075465

Updated

3 years ago
Duplicate of this bug: 1075530
You need to log in before you can comment on or make changes to this bug.