Closed Bug 1130084 Opened 5 years ago Closed 5 years ago

WiFi connection error even though it worked

Categories

(DevTools Graveyard :: WebIDE, defect)

defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

Attachments

(4 files, 2 obsolete files)

WebIDE shows an error, even though connection succeeds.
Somewhat related, I find the connection's sudden success confusing:

One moment WebIDE displays a dialog with a QR code that you're trying to get with your phone's camera feed, and the next all interfaces suddenly vanish (phone shows homescreen, WebIDE becomes blank apart from a discreet blue hint that connection was successful).

Maybe a brief "Connection successful!" confirmation screen before everything disappears could make the transition less confusing?
Jan, are you able to replicate the "error on success" state anymore?  I can't seem to do it now.
Flags: needinfo?(janx)
Attached image timedout.png
Yes, it still says "timed out" when QR scanning takes a long time, but the connection works anyway (I think I just did the scanning dance for 5 complete minutes to get this screenshot!)
Flags: needinfo?(janx)
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Attached file MozReview Request: bz://1130084/jryans (obsolete) —
/r/5699 - Bug 1130084 - Avoid spurious connection errors even on success. r=past

Pull down this commit:

hg pull review -r 00ed35911a16345fd5ccb81a63f92e9b83c19ab6
Attachment #8579763 - Flags: review?(past)
To encourage this bad case to happen, you can lower "devtools.debugger.remote-timeout" before connecting.  The default value (20000) meant you would see an error if the WiFi connection took more than 20 seconds to complete.

With this patch, we disable the timeout for WiFi connections.
Comment on attachment 8579763 [details]
MozReview Request: bz://1130084/jryans

https://reviewboard.mozilla.org/r/5697/#review4641

Ship It!
Attachment #8579763 - Flags: review?(past) → review+
Comment on attachment 8579763 [details]
MozReview Request: bz://1130084/jryans

Works like a charm! Thank you for this.
Attachment #8579763 - Flags: feedback?(janx) → feedback+
https://hg.mozilla.org/mozilla-central/rev/a244a42cfa0c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Attached image timeoutagain.png
The landed patch seems to help, but it looks like the bug isn't entirely fixed. I was able to reproduce this 2 out of 3 times when the scanning took a very, very long time.
Flags: needinfo?(jryans)
(In reply to Jan Keromnes [:janx] from comment #11)
> Created attachment 8580647 [details]
> timeoutagain.png
> 
> The landed patch seems to help, but it looks like the bug isn't entirely
> fixed. I was able to reproduce this 2 out of 3 times when the scanning took
> a very, very long time.

Hmm, yeah, I can see it sometimes too.  I'll try piling on more changes. :)
Status: RESOLVED → REOPENED
Flags: needinfo?(jryans)
Resolution: FIXED → ---
A second patch to clear out WebIDE's own global timeout.  I *think* this covers all cases now.

This probably suggests it's unnecessary to have two levels of timeouts... but I think I'll worry about that another day.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2af3eabcd6bb
Attachment #8581824 - Flags: review?(past)
Attachment #8581824 - Flags: feedback?(janx)
Comment on attachment 8581824 [details] [diff] [review]
0001-Bug-1130084-Allow-runtimes-to-take-infinite-time-to-.patch

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

::: browser/devtools/webide/modules/runtimes.js
@@ +65,5 @@
>   *   to be stable across Firefox sessions.
>   * |name| field
>   *   A user-visible label to identify the runtime that will be displayed in a
>   *   runtime list.
> + * |connectionIndeterminate| field

Nit: The name "connectionIndeterminate" is confusing. Please use something more understandable like "disableTimeout" or "noTimeout".

::: browser/devtools/webide/test/test_runtime.html
@@ +188,5 @@
> +          ok(win.document.querySelector("window").className, "busy", "UI is busy");
> +
> +          setTimeout(() => {
> +            noErrorDeferred.resolve();
> +          }, 1000);

It feels painful to make a test last 1000 seconds on purpose. Isn't there another way, e.g. checking if the busy timeout was indeed cancelled?
Attachment #8581824 - Flags: feedback?(janx) → feedback-
Sorry, I meant 1000 milliseconds. But maybe that's not too painful after all.
Comment on attachment 8581824 [details] [diff] [review]
0001-Bug-1130084-Allow-runtimes-to-take-infinite-time-to-.patch

I was able to try the patch, the time-out error seems to be gone, and the tests do not become painfully long.

As discussed on IRC, f+ if you change to the runtime field name to "prolongedConnection".
Attachment #8581824 - Flags: feedback- → feedback+
(In reply to Jan Keromnes [:janx] from comment #14)

> ::: browser/devtools/webide/test/test_runtime.html
> @@ +188,5 @@
> > +          ok(win.document.querySelector("window").className, "busy", "UI is busy");
> > +
> > +          setTimeout(() => {
> > +            noErrorDeferred.resolve();
> > +          }, 1000);
> 
> It feels painful to make a test last 1000 seconds on purpose. Isn't there
> another way, e.g. checking if the busy timeout was indeed cancelled?

Thankfully it's indeed just 1 second...  I know timeouts are bad, but I did not see a nice way to avoid it here, since the real feature under test is also based on a timeout.
Changed to "prolongedRuntime" as :janx suggested.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=932ad6adbf8f
Attachment #8581824 - Attachment is obsolete: true
Attachment #8581824 - Flags: review?(past)
Attachment #8581920 - Flags: review?(past)
Attachment #8581920 - Flags: feedback+
Comment on attachment 8581920 [details] [diff] [review]
Allow runtimes to take infinite time to connect (v2)

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

::: browser/devtools/webide/test/test_runtime.html
@@ +159,5 @@
>            ok(win.UI.toolboxIframe, "Toolbox iframe exists");
>  
>            yield win.Cmds.disconnectRuntime();
>  
> +          Services.prefs.setIntPref("devtools.webide.busyTimeout", 100);

Is this pref cleared anywhere afterwards?
Attachment #8581920 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #19)
> ::: browser/devtools/webide/test/test_runtime.html
> @@ +159,5 @@
> >            ok(win.UI.toolboxIframe, "Toolbox iframe exists");
> >  
> >            yield win.Cmds.disconnectRuntime();
> >  
> > +          Services.prefs.setIntPref("devtools.webide.busyTimeout", 100);
> 
> Is this pref cleared anywhere afterwards?

Yes, I added it to a general pref clearing blocking in head.js for the folder.
https://hg.mozilla.org/mozilla-central/rev/0362eb765103
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Attachment #8579763 - Attachment is obsolete: true
Attachment #8619334 - Flags: review+
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.