Opened 6 months ago

Closed 6 months ago

Last modified 5 weeks ago

#23240 closed defect (fixed)

do not show progress bar at zero when bootstrap progress is greater than zero

Reported by: mcs Owned by: brade
Priority: Medium Milestone:
Component: Applications/Tor Launcher Version:
Severity: Normal Keywords: TorBrowserTeam201708R, tbb-not-backported
Cc: brade, tbb-team, dcf, linda Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor4

Description

To help with some of the usability issues related to #22266, we should avoid showing the bootstrap progress bar in Tor Launcher at zero when the actual phase/progress value is greater than zero. Therefore we will change Tor Launcher to retrieve the current bootstrap progress value before displaying its progress bar. That way users will not see a surprising jump from 0% to 80%.

Child Tickets

Change History (11)

comment:1 Changed 6 months ago by mcs

Cc: tbb-team added
Keywords: TorBrowserTeam201708R added
Status: newneeds_review

Here is an oniongit merge request to use for code review and discussion:
https://oniongit.eu/brade/tor-launcher/merge_requests/1
(comments here are welcome too).

comment:2 Changed 6 months ago by dcf

Cc: dcf added

comment:1 looks like an effective workaround, but I suspect it can be addressed in a more direct way. I think the root of the problem is that progressMeter.value is reset to 0 every time the progress bar dialog is shown. I added some logging:

--- a/src/chrome/content/progress.js
+++ b/src/chrome/content/progress.js
@@ -51,6 +51,10 @@ function initDialog()
   }
   catch (e) {}
 
+  var meter = document.getElementById("progressMeter");
+  if (meter)
+    TorLauncherLogger.log(6, "initDialog meter.value " + meter.value);
+
   var isBrowserStartup = false;
   if (window.arguments)
   {
@@ -181,9 +185,12 @@ var gObserver = {
       var labelText =
                 TorLauncherUtil.getLocalizedBootstrapStatus(statusObj, "TAG");
       var percentComplete = (statusObj.PROGRESS) ? statusObj.PROGRESS : 0;
+      TorLauncherLogger.log(6, "statusObj.PROGRESS " + statusObj.PROGRESS);
 
       var meter = document.getElementById("progressMeter");
+      if (meter)
+        TorLauncherLogger.log(6, "meter.value " + meter.value + " -> " + percentComplete);
       if (meter)
         meter.value = percentComplete;
 
       var bootstrapDidComplete = (percentComplete >= 100);

I hacked Browser/start-tor-browser to add --jsconsole to the command line so I could see the log output. Then I did this:

  1. ./start-tor-browser.desktop
  2. Click "Open Settings" quickly before the bootstrap finishes.
  3. Wait a few seconds on the initial screen.
  4. Click "Connect" to make the progress bar reappear and let it finish bootstrapping.

Step 4 of the above process shows the problem from #22266: even though the bootstrapping is really at ~90%, the progress bar goes back to 0% and then straight to 100% the second time the progress dialog is shown.

Here is the log output:

[08-15 01:41:13] TorLauncher -: initDialog meter.value 0
Tor NOTICE: Bootstrapped 85%: Finishing handshake with first hop 
[08-15 01:41:14] TorLauncher -: statusObj.PROGRESS 85
[08-15 01:41:14] TorLauncher -: meter.value 0 -> 85
Tor NOTICE: Bootstrapped 90%: Establishing a Tor circuit 
[08-15 01:41:14] TorLauncher -: statusObj.PROGRESS 90
[08-15 01:41:14] TorLauncher -: meter.value 85 -> 90

  (Here is where I clicked "Open Settings", waited a few seconds, then clicked "Connect")

[08-15 01:41:20] TorLauncher -: initDialog meter.value 0
Tor NOTICE: Bootstrapped 100%: Done 
[08-15 01:41:21] TorLauncher -: statusObj.PROGRESS 100
[08-15 01:41:21] TorLauncher -: meter.value 0 -> 100

Notice that when initDialog is called the second time, meter.value is reset to 0, rather than remembering its previous value of 90. Maybe this is because openProgressDialog creates a brand new dialog from scratch? Perhaps instead, it could keep the same dialog in memory, and show or hide it as appropriate, so that meter.value retains its state.

I don't think the usability problem from #22266 is an initial jump from 0% to 80%. That's normal and expected with an up-to-date consensus cache. Rather, the usability problem is when you cancel an in-progress bootstrapping at 60%, and when you resume, the meter goes down to 0% then back up to 65%. The changes in comment:1 make the bar invisible when it would misleadingly be set at 0%, but it would be better if the bar remembered its state and didn't go back to 0% at all.

comment:3 Changed 6 months ago by dcf

That said, I do think the call to protocolSvc.TorRetrieveBootstrapStatus() is a good idea in any case. I believe the safety timeout is not necessary if the progress bar keeps state and remains unhidden.

comment:4 in reply to:  2 Changed 6 months ago by mcs

Replying to dcf:

comment:1 looks like an effective workaround, but I suspect it can be addressed in a more direct way. I think the root of the problem is that progressMeter.value is reset to 0 every time the progress bar dialog is shown. I added some logging:
...
Notice that when initDialog is called the second time, meter.value is reset to 0, rather than remembering its previous value of 90. Maybe this is because openProgressDialog creates a brand new dialog from scratch? Perhaps instead, it could keep the same dialog in memory, and show or hide it as appropriate, so that meter.value retains its state.

Thanks for looking at this problem. Kathy and I considered but decided against caching the current value. We think it is more elegant — and safer — for Tor Launcher to be stateless with respect to the progress value. For example, even if tor needs to reset its notion of progress due to a restart or because of radical reconfiguration, the Tor Launcher progress bar will never show an incorrect value.

I don't think the usability problem from #22266 is an initial jump from 0% to 80%. That's normal and expected with an up-to-date consensus cache.

Agreed, although I think it is better to hide that jump (progress isn't really zero so why show it as zero?)

Other opinions?

comment:5 Changed 6 months ago by mcs

Cc: linda added

comment:6 Changed 6 months ago by gk

Just for posterity

14:39 <+GeKo> we should do the following:
14:39 <+GeKo> a) show the current progress as accurate as we can
14:39 <+GeKo> and
14:40 <+GeKo> b) only jump from X to Y if there is actually a progress from X to Y

comment:7 Changed 6 months ago by gk

Status: needs_reviewneeds_information

The code works for me and seems to fix the things mentioned in comment:6. mcs/brade: I'd be happy merging it into master, closing this ticket, and giving it some testing in the next alpha. What do you think?

comment:8 in reply to:  7 ; Changed 6 months ago by mcs

Replying to gk:

The code works for me and seems to fix the things mentioned in comment:6. mcs/brade: I'd be happy merging it into master, closing this ticket, and giving it some testing in the next alpha. What do you think?

Sure, let's try it in the next alpha.

comment:9 in reply to:  8 Changed 6 months ago by gk

Resolution: fixed
Status: needs_informationclosed

Replying to mcs:

Replying to gk:

The code works for me and seems to fix the things mentioned in comment:6. mcs/brade: I'd be happy merging it into master, closing this ticket, and giving it some testing in the next alpha. What do you think?

Sure, let's try it in the next alpha.

Done. Cherry-picked to master with commit 26d284e3835ddd5b283b732278b7b5b740bace6e.

comment:10 Changed 6 months ago by gk

Keywords: tbb-backport added

comment:11 Changed 5 weeks ago by gk

Keywords: tbb-not-backported added; tbb-backport removed

Not backported but will be available in 7.5.

Note: See TracTickets for help on using tickets.