This when connecting to a new WIFI AP, the detection should be performed immediately. Bug 1332271 comment 17

MozReview-Commit-ID: L8XBLx88PbS

I've pushed this to try for Adrian's benefit, builds will be available here when ready: https://archive.mozilla.org/pub/firefox/try-builds/nhnt11@gmail.com-457329d496894cdcb18ba66309698bb81b524224/

https://hg.mozilla.org/integration/mozilla-inbound/rev/328536bee4f5d9ed12019af804dc78218064fb6d Bug 1334520 - Trigger captive portal recheck recheck for NS_NETWORK_LINK_DATA_CHANGED events r=mcmanus

(In reply to Nihanth Subramanya [:nhnt11] from comment #2) > I've pushed this to try for Adrian's benefit, builds will be available here > when ready: > > https://archive.mozilla.org/pub/firefox/try-builds/nhnt11@gmail.com- > 457329d496894cdcb18ba66309698bb81b524224/ I've tried on Windows 10 x64 and I can't reproduce it. I didn't saw any try build for ubuntu, this problem was also confirmed on Ubuntu.

(In reply to ovidiu boca[:Ovidiu] from comment #4) > (In reply to Nihanth Subramanya [:nhnt11] from comment #2) > > I've pushed this to try for Adrian's benefit, builds will be available here > > when ready: > > > > https://archive.mozilla.org/pub/firefox/try-builds/nhnt11@gmail.com- > > 457329d496894cdcb18ba66309698bb81b524224/ > > I've tried on Windows 10 x64 and I can't reproduce it. I didn't saw any try > build for ubuntu, this problem was also confirmed on Ubuntu. Weird! That link should have all builds. Anyway, here are the linux builds: Linux: https://queue.taskcluster.net/v1/task/aVGkVRPJQgaexukrZAdw-w/runs/0/artifacts/public/build/target.tar.bz2 Linux64: https://queue.taskcluster.net/v1/task/bOxRemMVT3muhCw5PrrXDA/runs/0/artifacts/public/build/target.tar.bz2

Adrian, do you think this is a requirement in our 52 release? Let me know if I should request uplift. Thanks!

IMHO, this should be indeed uplifted to 52/53, but before you do, let me test on Linux as well. I think we might have a regression related to this fix.

(In reply to Adrian Florinescu [:AdrianSV] [PTO jan 27 -feb 01] from comment #8) > IMHO, this should be indeed uplifted to 52/53, but before you do, let me > test on Linux as well. I think we might have a regression related to this > fix. Actually, I'm not sure if this is a regression or not and the reproducibility of it is highly intermittent: sometimes(1 of 20-30 tries), when I disconnect the Wi-fi, the Captive Portal notification bar remains and it is not dismissed. My thoughts on this are that we might have a very narrow edge case in which the Wifi disconnects somehow silently and FF doesn't get that the state has changed. But like I said, I have no reproduction steps for it and the reproducibility rate is extremely low at the moment. Summing up, I see no value at this moment in logging a new bug with the above behavior, and since dismissing the CP notification bar is basic functionality for this feature we have test cases that will catch this behavior if it becomes reproducible. Nihanth or Valentin what do you think about the above edge case from the code point of view? Fix verified on latest Nightly (54.0a1/2017-01-31) on Ubuntu 16.04 as well. Related to the uplift, I think that this is a high value change for Captive Portal feature that we should have in 53/52.

(In reply to Adrian Florinescu [:AdrianSV] [PTO jan 27 -feb 01] from comment #9) > Actually, I'm not sure if this is a regression or not and the > reproducibility of it is highly intermittent: sometimes(1 of 20-30 tries), > when I disconnect the Wi-fi, the Captive Portal notification bar remains > and it is not dismissed. My thoughts on this are that we might have a very > narrow edge case in which the Wifi disconnects somehow silently and FF > doesn't get that the state has changed. > But like I said, I have no reproduction steps for it and the reproducibility > rate is extremely low at the moment. > > Summing up, I see no value at this moment in logging a new bug with the > above behavior, and since dismissing the CP notification bar is basic > functionality for this feature we have test cases that will catch this > behavior if it becomes reproducible. Nihanth or Valentin what do you think > about the above edge case from the code point of view? This is pretty weird, I checked the frontend code and I don't see any reason the notification bar wouldn't be removed as long as we receive the success/abort observer notification (there are sufficient null checks). Assuming I'm right, that means the captive portal state change doesn't get propagated, which means an issue with captivedetect.js. Or - and this feels less likely to me - there might be a necko issue here. I'm inclined to agree that we can file this if/when it becomes reproducible.

One additional note that I somehow forgot to mention in comment 9, the issue we are discussing about (CP notification not being dismissed on disconnect) has been observed only under Linux environment (we've observed no such behavior in win/osx), that's why initially I said in comment 8 that this might be a regression.

Comment on attachment 8831168 [details] [diff] [review] Trigger captive portal recheck recheck for NS_NETWORK_LINK_DATA_CHANGED events Approval Request Comment [Feature/Bug causing the regression]: Bug present since the initial captive portal implementation [User impact if declined]: It could take a long time until the CP is detected. [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: see bug 1332271 comment 0 [List of other uplifts needed for the feature/fix]: other patches already uplifted [Is the change risky?]: not risky [Why is the change risky/not risky?]: It only triggers a CP detection for a relevant network event. [String changes made/needed]: none

Comment on attachment 8831168 [details] [diff] [review] Trigger captive portal recheck recheck for NS_NETWORK_LINK_DATA_CHANGED events Fix a detection issue when connecting to a new WIFI AP. Aurora53+. Per comment #9, take this in aurora first and see if any regressions happen.

Comment on attachment 8831168 [details] [diff] [review] Trigger captive portal recheck recheck for NS_NETWORK_LINK_DATA_CHANGED events check captive portal status on network link change event, beta52+

Verified fixed on Windows 10 x64, Mac OS X 10.11 and Ubuntu 14.04 x64 using Firefox 52 Beta 9 (buildID: 20170223185858) and latest Aurora 53.0a2 (2017-02-24).