Clicking Browse button in file input kills WebSocket connections

RESOLVED FIXED in Firefox 54

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: David Jameson, Assigned: Grisha)

Tracking

50 Branch
Firefox 54
Points:
---

Firefox Tracking Flags

(fennec+, firefox54 fixed)

Details

(Whiteboard: [MobileAS])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36

Steps to reproduce:

(1) Add an input type "file"
(2) Open a Websocket connection
(3) Click on the Browse button (in the file input)
(4) Either choose a file, or press the back button to exit the file chooser
(5) The WebSocket connection closes within 5 or 10 seconds

Might be related to https://bugzilla.mozilla.org/show_bug.cgi?id=858538

I don't have a minimal example yet.
This may be the Android activity monitor killing us in the background. If enabling the Android developer settings and setting don't keep activities makes this 100% reproducible the Android activity lifecycle will need to be worked around.
tracking-fennec: --- → ?
(Reporter)

Comment 2

9 months ago
It is already 100% reproducible for me even without that option set.

Right now you can see it happening at:

https://www.groupworld.net/new_conference_js.html

Click NO when asked to use the app, then enter any name, and click the upload icon (leftmost one at the top), then cancel the file selector. You'll see after about 5 secs you get a lost connection, which is happening on the client side (but not initiated by us). Works fine on Android Chrome, and also on Firefox running on PC/Mac.

It should be easy to create a minimal example if that will help.
Hey Jim,
Do you have any idea about this bug?
Flags: needinfo?(nchen)
I think this could be a regression from bug 1236130. I don't see the problem if I comment out the call to GeckoNetworkManager.stop() [1]. I think the issue is we're forcing Gecko to reset the network every time we pause/resume (by stopping/starting GeckoNetworkManager and then updating the network state).

[1] http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#127
Flags: needinfo?(nchen) → needinfo?(gkruglov)
(Assignee)

Comment 5

9 months ago
Strange. When activity is paused, GeckoNetworkManager unregisters broadcast receiver for the "connectivity changed" intents. When it's resumed, it registers a receiver. It switches its internal state from OnWithListeners to OffWithListeners and vice versa. See [0] and friends. So its not clearing any internal state (current connectivity info, etc), nor is it sending any network state updates to Gecko on either stop or receive event; it just stops listening for those updates from the OS while activity isn't active.

[0] https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoNetworkManager.java?q=path%3AGeckoNetworkManager.java&redirect_type=single#281-283
Flags: needinfo?(gkruglov)
(Assignee)

Comment 6

9 months ago
What _does_ happen once we re-register for "connectivity changed" intents, is that we immediately receive one. In case connectivity didn't change, interestingly enough, we end up dispatching a onStatusChanged("changed") call to gecko in [0]. This seem strange to me, to explicitly say that network status change when it didn't change, and I can't remember if there was a good reason to do that. Most likely it was done this way to correspond to GeckoNetworkManager's pre-state-machine-rewrite behaviour.

Jim, perhaps that "changed" network update is causing problems on the receiving end? I'm happy to change this behaviour and not send a "changed" status update when nothing, in fact, changed.

[0] https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoNetworkManager.java?q=path%3AGeckoNetworkManager.java&redirect_type=single#350
(In reply to :Grisha Kruglov from comment #6)
> I'm happy to change this behaviour and not send a "changed"
> status update when nothing, in fact, changed.

Makes sense. I suppose the network could change to one state and then change back to the original state while we're in the background, but it's unclear to me if Gecko will be affected if we don't notify it of the change/change-back.
(Assignee)

Comment 8

9 months ago
According to [0], "changed" means that "link up is still true, but network setup was modified". Given that network setup wasn't modified in a case of pausing/resuming an activity, we shouldn't be sending that event according to that definition.

Unless GeckoNetworkManager keeps track of "i just turned on broadcast listening, so I'm expecting to see an intent w/ network info, which might or might not be the same as my current network info" (which is a hack that will likely cause us to miss network state changes), it can't know when _not_ to send a "changed" event if nothing seemingly changed. Connection type/subtype that we keep around is fairly coarse (see [3] and Bug 1270401). So I guess it's possible we might get updates from the OS which will look like nothing changed from our perspective, but network setup did indeed change in some way. That seems like a reason why we're sending "changed" in `sendNetworkStateToListeners` as a default action if we can't figure out what happened.

Not sending "changed" if connection type/subtype and network status didn't obviously change does seem to "fix" the issue, as in scenario described in Comment 2 stops happening.

Looking lower in the stack, when WebSocketChannel receives a "false" networkChanged call it forces a ping if there isn't an outstanding ping already... Perhaps some of that flow gets tangled up somehow? See [1], [2].

[0] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsINetworkLinkService.idl#67-71
[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#1332
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/websocket/WebSocketChannel.cpp#3262
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/NetworkUtils.java#20
(Assignee)

Comment 9

9 months ago
The "do not send 'changed' when nothing changes as far as G.N.M. can tell" patch should be trivial. I suppose we could land it and see if there are any adverse effects? ;)
Assignee: nobody → gkruglov
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

9 months ago
Iteration: --- → 1.14
Priority: -- → P1
Whiteboard: [MobileAS]
Comment hidden (mozreview-request)
(Assignee)

Comment 11

9 months ago
(In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> Makes sense. I suppose the network could change to one state and then change
> back to the original state while we're in the background, but it's unclear
> to me if Gecko will be affected if we don't notify it of the
> change/change-back.

We also wouldn't know about this change/change-back state on the Java side, since while we're in the background, we're not listing for any network status updates.

Comment 12

9 months ago
mozreview-review
Comment on attachment 8832526 [details]
Bug 1330836 - Only send 'network config/status changed' events to Gecko if something actually changed

https://reviewboard.mozilla.org/r/108768/#review110020

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoNetworkManager.java:353
(Diff revision 1)
>          final String status;
>  
> -        if (currentNetworkStatus != previousNetworkStatus) {
> +        // As far as we can tell, network configuration did not change. Connection type, subtype and
> +        // status are still the same. We choose to not send a "changed" notification in this case.
> +        // See Bug 1330836 for relevant details.
> +        if (currentNetworkStatus == previousNetworkStatus) {

With this patch, if the connection changes from "wifi up" to "4g up", the status is still "up", so we don't send status changed, but I think we still should. That is, only skip status changed when connection type did not change either. (That's what your comment implied but I only see the status change comparison here.)

I'm not too familiar with this code so let me know if I'm reading that wrong.
Attachment #8832526 - Flags: review?(nchen)
(Assignee)

Comment 13

9 months ago
mozreview-review
Comment on attachment 8832526 [details]
Bug 1330836 - Only send 'network config/status changed' events to Gecko if something actually changed

https://reviewboard.mozilla.org/r/108768/#review110034

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoNetworkManager.java:353
(Diff revision 1)
>          final String status;
>  
> -        if (currentNetworkStatus != previousNetworkStatus) {
> +        // As far as we can tell, network configuration did not change. Connection type, subtype and
> +        // status are still the same. We choose to not send a "changed" notification in this case.
> +        // See Bug 1330836 for relevant details.
> +        if (currentNetworkStatus == previousNetworkStatus) {

You're correct. This will send `onConnectionChanged` in that case, and for some reason I thought it would be enough - but that is the exact case "changed" should cover (connection still up, and its config is different), so we should be sending it as well. I'll update the patch.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

8 months ago
mozreview-review
Comment on attachment 8832526 [details]
Bug 1330836 - Only send 'network config/status changed' events to Gecko if something actually changed

https://reviewboard.mozilla.org/r/108768/#review111204

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoNetworkManager.java:362
(Diff revision 3)
>  
> +        // Something changed, so send a "changed" status along with new network status state.
> +        final String status = currentNetworkStatus.value;
> +
>          if (GeckoThread.isRunning()) {
> +            onStatusChanged(LINK_DATA_CHANGED);

I'm not sure if this is strictly necessary. Semantics of these calls imply that we should be sending "changed", but it's unclear to me when.

Current patch sends "changed" if something changed, along with the new current state.

so,
"changed" + "down"
"changed" + "up"
"changed" + "unknown"

are all possible combinations.

Comment 17

8 months ago
mozreview-review
Comment on attachment 8832526 [details]
Bug 1330836 - Only send 'network config/status changed' events to Gecko if something actually changed

https://reviewboard.mozilla.org/r/108768/#review111162

r+ but see comments below.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoNetworkManager.java:48
(Diff revision 3)
>   * This class depends on access to the context, so only use after GeckoAppShell has been initialized.
>   */
>  public class GeckoNetworkManager extends BroadcastReceiver implements BundleEventListener {
>      private static final String LOGTAG = "GeckoNetworkManager";
>  
> +    // While gecko's network interfaces define this event, currently we do not use it.

Probably need a new comment here

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoNetworkManager.java:362
(Diff revision 3)
>  
> +        // Something changed, so send a "changed" status along with new network status state.
> +        final String status = currentNetworkStatus.value;
> +
>          if (GeckoThread.isRunning()) {
> +            onStatusChanged(LINK_DATA_CHANGED);

Comment in native code says "CHANGED means UP/DOWN didn't change but the status of the captive portal may have changed" [1].

That suggests to me we should send "changed" if `(previousNetworkStatus == currentNetworkStatus && connectionTypeOrSubtypeChanged)`, and send "up"/"down"/"unknown" if `previousNetworkStatus != currentNetworkStatus`. That is the same as the previous behavior, so I would keep the `if (currentNetworkStatus != previousNetworkStatus) {` block that you removed, and only call `onStatusChanged` once.

[1] http://searchfox.org/mozilla-central/rev/f5077ad52f8b90183e73038869f6140f0afbf427/netwerk/base/nsIOService.cpp#1625
Attachment #8832526 - Flags: review?(nchen) → review+

Updated

8 months ago
Iteration: 1.14 → 1.15
tracking-fennec: ? → +
Comment hidden (mozreview-request)

Comment 19

8 months ago
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b29057e11bc0
Only send 'network config/status changed' events to Gecko if something actually changed r=jchen

Comment 20

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b29057e11bc0
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.