Opened 3 weeks ago

Closed 3 weeks ago

#27403 closed defect (fixed)

The onboarding bubble does not show up in the release candidate for Tor Browser

Reported by: gk Owned by: tbb-team
Priority: Immediate Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201808R
Cc: arthuredelstein, mcs, brade, boklm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

On start the onboarding bubble does not show up on my Linux box at least. Arthur can reproduce that and says that any resizing makes it appear.

Child Tickets

Attachments (1)

possible-fix.patch (1.3 KB) - added by mcs 3 weeks ago.
a possible fix (needs work)

Download all attachments as: .zip

Change History (12)

comment:1 Changed 3 weeks ago by gk

Again the last good nightly is from 08/27 and the first bad one from 08/30. Reverting the security slider notification related commit (1eb701f4701340c89c9f76ad2eb6ae86ca051e61) did not help. Neither did reverting the one that improved the about:tor behavior/appearance (3bf17f3f9ca7fd40ed13d40f628384d38ba71368). Resizing does not show up the bubble for me.

comment:2 Changed 3 weeks ago by mcs

I am not 100% sure that the root cause is the same as this bug, but sometimes the onboarding bubble is not shown when I repeatedly press Cmd+T on macOS after the first about:tor opens (to get more about:tor tabs). I traced the problem to a zero window width inside the _resizeUI() function within browser/extensions/onboarding/content/onboarding.js.

Interestingly, the problem disappears if I comment out the following inside Torbutton's src/chrome/skin/aboutTor.css file:

body:not([initialized]) {
  display: none;
}

My theory is that there is a race where sometimes the onboarding extension's _resizeUI() function runs before about:tor has set the initialized attribute on the body (so the document size is detected as zero).

One possible way to fix this is to arrange for _resizeUI() to be called a second (or third or ...) time if the document has a size of zero. I will attach a *very* rough patch that fixes the problem for me. I am out of time for tonight, but maybe Arthur or Georg can clean it up or find a better solution. It would also be good to confirm that this is what is occurring on Linux with the Tor Browser 8 RC.

Changed 3 weeks ago by mcs

Attachment: possible-fix.patch added

a possible fix (needs work)

comment:3 Changed 3 weeks ago by mcs

I forgot to mention that I do not know why this is just showing up now, but if it is a race maybe the recent NoScript changes or something else made it worse?

comment:4 Changed 3 weeks ago by gk

Thanks! So, we might have two different issues here (which could easily be related): your fix (and commenting out the aboutTor.css pieces) "solves" the problem in subsequent Tor Browser starts but not on first start. On first start for some interesting reason _resizeUI() is never called. (I don't see your debug output in my logs in that case but do in the following Tor Browser starts)

comment:5 Changed 3 weeks ago by arthuredelstein

Keywords: TorBrowserTeam201808R added
Status: newneeds_review

Here's my fix, also not fully tested. It worked on one machine at least.

https://github.com/arthuredelstein/tor-browser/commit/27403

I found that this._window.document.body.getBoundingClientRect().width; is returning zero early on, but this._window.innerWidth returns the correct width of the window.

comment:6 in reply to:  4 Changed 3 weeks ago by mcs

Replying to gk:

Thanks! So, we might have two different issues here (which could easily be related): your fix (and commenting out the aboutTor.css pieces) "solves" the problem in subsequent Tor Browser starts but not on first start. On first start for some interesting reason _resizeUI() is never called. (I don't see your debug output in my logs in that case but do in the following Tor Browser starts)

I debugged this a little more. The two problems seem to be unrelated. What is happening in the first run case is that the onboarding extension is not yet listening for load events when about:tor loads. The relevant code is in browser/extensions/onboarding/bootstrap.js. The browser-delayed-startup-finished observer notification is not received soon enough. I tried switching to final-ui-startup but that did not help. Unfortunately, I am out of time again for now.

comment:7 in reply to:  5 Changed 3 weeks ago by gk

Replying to arthuredelstein:

Here's my fix, also not fully tested. It worked on one machine at least.

https://github.com/arthuredelstein/tor-browser/commit/27403

I found that this._window.document.body.getBoundingClientRect().width; is returning zero early on, but this._window.innerWidth returns the correct width of the window.

This works for me after second start. The first start issue is still not solved.

comment:8 Changed 3 weeks ago by mcs

Arthur's idea of checking against innerWindow is a good one.

Here is a patch that fixes both problems for me on macOS:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug27403-01&id=6eff180c3fa1d750828e355726e12d4fbec228bb

I now have to disappear for the rest of today and won't have time to make Linux and Windows builds. Please test!

comment:9 in reply to:  8 ; Changed 3 weeks ago by gk

Replying to mcs:

Arthur's idea of checking against innerWindow is a good one.

Here is a patch that fixes both problems for me on macOS:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug27403-01&id=6eff180c3fa1d750828e355726e12d4fbec228bb

I now have to disappear for the rest of today and won't have time to make Linux and Windows builds. Please test!

Works for me on my Linux machine, nice! I am a bit confused about your patch and your comment:6. In the patch you are using final-up-startup but in the comment you said it did not work for you. What's up with that? I checked that profile-after-change works (as well) which might be a suitable fallback.

comment:10 in reply to:  9 ; Changed 3 weeks ago by mcs

Replying to gk:

Works for me on my Linux machine, nice! I am a bit confused about your patch and your comment:6. In the patch you are using final-up-startup but in the comment you said it did not work for you. What's up with that?

I forgot that I had mentioned that in comment:6. Sorry! I think I forgot to copy my newly built browser into my test area before I did that test earlier. I caught myself making that mistake later, so I probably did earlier too.

Last edited 3 weeks ago by mcs (previous) (diff)

comment:11 in reply to:  10 Changed 3 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to mcs:

Replying to gk:

Works for me on my Linux machine, nice! I am a bit confused about your patch and your comment:6. In the patch you are using final-up-startup but in the comment you said it did not work for you. What's up with that?

I forgot that I had mentioned that in comment:6. Sorry! I think I forgot to copy my newly built browser into my test area before I did that test earlier. I caught myself making that mistake later, so I probably did earlier too.

Makes sense and no worries. Thanks for your and Arthur's help. I picked up your patch from bug27403-01 as it is the simplest one we found for fixing both issues (commit cd00a313475584c0a4b94eaea5171428b993ce5b). I added a small fixup to it for displaying the onboarding bubble on smaller displays even with width < 960px available which Arthur suggested (commit 839cc4bd054821fe7e4c093f0a05986b83491985). Before that I mistakenly pushed an older proposed bugfix by Arthur (8b50d3f2f1bb5ce04ebf3a91ff701059fedb2da2) which I reverted with commit c79bb726065ce296d567a6c9ac58b37b7d750318. All this happened on tor-browser-60.2.0esr-8.0-1.

I think we are done here now. \o/

Note: See TracTickets for help on using tickets.