Opened 3 years ago

Closed 3 years ago

Last modified 9 months ago

#16940 closed enhancement (fixed)

Implement loading (only) local change notes after a Tor Browser update

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-5.5, TorBrowserTeam201511R, tbb-no-uplift
Cc: mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In #13512 we implemented showing change notes after a Tor Browser update. This is essentially done by loading the blog post on first start after the update. There were some concerns mainly over the reliability of this procedure which led to the idea to implement parts or all of this mechanism using local resources.

Child Tickets

Attachments (1)

about-tbupdate.png (423.8 KB) - added by mcs 3 years ago.
prototype of about:tbupdate page

Download all attachments as: .zip

Change History (18)

comment:1 Changed 3 years ago by gk

Quoting comment:32:ticket:13512:

Replying to gk:

One final thing I am wondering is whether we really should contact the blog or an external resource after an update. If that goes wrong (mybe there is an issue with the server, or whatever) the users are seeing a weird error message instead of the site we want to show them and instead of the familiar about:tor page (which is now a backgounrd tab). And we put additional load on the Tor network (an on the server hosting the blog) instead of using the changelog we ship with the browser. These concerns might be for a follow-up bug, though (while we are testing the things we have so far in the alpha cycle).

I am not sure how concerned we should be about the network load but if an error occurs while loading the page, that could be confusing. We could create a local page (about:update or similar) that just said "An update has been applied" which also included a link (or an iframe) that pointed to a remote page. We would need to figure out how to pass the remote link to the about:update page.

Alternatively, we could create a local page that displayed the top part of ChangeLog.txt. But doing that would take away a major benefit of loading a remote page: no ability to inform the user about issues and/or workarounds we learn after release.

Or we could combine both approaches. Any approach that reads ChangeLog.txt would require us to standardize the format of that file (e.g., a blank line separates the info for one versions from the next).

Last edited 3 years ago by gk (previous) (diff)

Changed 3 years ago by mcs

Attachment: about-tbupdate.png added

prototype of about:tbupdate page

comment:2 Changed 3 years ago by mcs

Severity: Normal
Status: newneeds_information

Kathy and I created a prototype of a local about:tbupdate page that pulls in text from Docs/ChangeLog.txt and also includes a link to a blog entry. Please take a look and provide feedback.

prototype of about:tbupdate page

comment:3 Changed 3 years ago by gk

I like it!

comment:4 Changed 3 years ago by mcs

Status: needs_informationneeds_review

We now have an implementation. Please review.
Part 1 is in tor-browser:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16940-01&id=0d181256ab1c42caf75108ef193f31eeee2ad961

Part 2 is in torbutton (localizable strings):
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug16940-01&id=9b800ffc669f7eb7dd6203a24aa85a39b5edd154

We will also need to add the aboutTBUpdate.dtd file to Transifex; the changes we made to import-translations.sh in torbutton assume that the new translations.git branch will be named torbutton-abouttbupdatedtd.

comment:5 Changed 3 years ago by gk

I wonder whether having linkPrefix and linkLabel that way is causing trouble for translators. My gut tells me "yes". The languages I am able to speak "no". Hm...

comment:6 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201511R added

comment:7 in reply to:  5 ; Changed 3 years ago by mcs

Replying to gk:

I wonder whether having linkPrefix and linkLabel that way is causing trouble for translators. My gut tells me "yes". The languages I am able to speak "no". Hm...

I am not sure, but this approach does provide flexibility, assuming the localizers are not confused by it. We did something very similar for the about:tor page (see aboutTor.failure3prefix.label, aboutTor.failure3Link, and aboutTor.failure3suffix.label within aboutTor.dtd). So far it looks like no one has needed to change the suffix (".").

comment:8 in reply to:  7 ; Changed 3 years ago by gk

Replying to mcs:

Replying to gk:

I wonder whether having linkPrefix and linkLabel that way is causing trouble for translators. My gut tells me "yes". The languages I am able to speak "no". Hm...

I am not sure, but this approach does provide flexibility, assuming the localizers are not confused by it. We did something very similar for the about:tor page (see aboutTor.failure3prefix.label, aboutTor.failure3Link, and aboutTor.failure3suffix.label within aboutTor.dtd). So far it looks like no one has needed to change the suffix (".").

Okay, sounds like it is working that way, fine with me.

I've tested the patches on Linux and local change notes are working + the code looks good. Nice job! Just two things occurred to me while testing/reading the code:

1) Should we prepend the value for startup.homepage_override_url with about:tbupdate? as well?
2) I wonder if we should update the comment above this preference to explain what is going on as we don't do this anywhere as far as I can see. And using openURL gives the wrong impression as we won't open the one mentioned there usually. Thus, folks might be confused. If not there, were else could mention the "hijacking" of the openURL parameter?

comment:9 in reply to:  8 ; Changed 3 years ago by mcs

Replying to gk:

I've tested the patches on Linux and local change notes are working + the code looks good. Nice job! Just two things occurred to me while testing/reading the code:

1) Should we prepend the value for startup.homepage_override_url with about:tbupdate? as well?

about:tbupdate? will be prepended to the overridePage whenever the browser is starting immediately after applying an update (app.update.postupdate). After the call to getPostUpdateOverridePage(), the overridePage value might be the startup.homepage_override_url, the value from the update manifest (XML from the server), or an empty string. So about:tbupdate? will be added to startup.homepage_override_url. Or are we missing something?

2) I wonder if we should update the comment above this preference to explain what is going on as we don't do this anywhere as far as I can see. And using openURL gives the wrong impression as we won't open the one mentioned there usually. Thus, folks might be confused. If not there, were else could mention the "hijacking" of the openURL parameter?

We will update the comment.

comment:10 in reply to:  9 ; Changed 3 years ago by gk

Replying to mcs:

Replying to gk:

I've tested the patches on Linux and local change notes are working + the code looks good. Nice job! Just two things occurred to me while testing/reading the code:

1) Should we prepend the value for startup.homepage_override_url with about:tbupdate? as well?

about:tbupdate? will be prepended to the overridePage whenever the browser is starting immediately after applying an update (app.update.postupdate). After the call to getPostUpdateOverridePage(), the overridePage value might be the startup.homepage_override_url, the value from the update manifest (XML from the server), or an empty string. So about:tbupdate? will be added to startup.homepage_override_url. Or are we missing something?

I tested the update changes by applying a MAR-file by hand and then the currently specified URL got loaded without about:tbupdate? prepended. I guess there are people with a bunch of tinfoil-hats out there doing this kind of thing regularly, so I was wondering.

comment:11 in reply to:  10 Changed 3 years ago by mcs

Replying to gk:

I tested the update changes by applying a MAR-file by hand and then the currently specified URL got loaded without about:tbupdate? prepended. I guess there are people with a bunch of tinfoil-hats out there doing this kind of thing regularly, so I was wondering.

Ah. Kathy and I had not considered that scenario. I think we will remove the app.update.postupdate pref. check; doing so will cause about:tbupdate to be opened whenever a restart occurs after new bits have been installed. I think about:tbupdate will also be opened in a downgrade situation, but that is probably OK too (and downgrades should only happen if someone manually applies an update or if they modify hidden prefs such as browser.startup.homepage_override.mstone and browser.startup.homepage_override.torbrowser.version).

comment:12 Changed 3 years ago by mcs

Please review the following revised tor-browser patch:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16940-02&id=4a726fc8a675c0882cbfba2514deb32b53ade2ab
(compared to the previous patch, we only made changes inside browser/components/nsBrowserContentHandler.js)

The torbutton patch is the same as before:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug16940-02&id=4a726fc8a675c0882cbfba2514deb32b53ade2ab.

comment:13 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks, merged to master (9b800ffc669f7eb7dd6203a24aa85a39b5edd154) and tor-browser-38.4.0esr-5.5-1 (4a726fc8a675c0882cbfba2514deb32b53ade2ab).

comment:14 Changed 3 years ago by mcs

See also #17729 (need to add aboutTBUpdate.dtd to Transifex).

comment:15 Changed 3 years ago by cypherpunks

This doesn't work on my Win7 x64. After Changelog: is empty.

Unless it is a bug, it might be due to manual hardening using EMET and similar tools on my side

comment:16 Changed 3 years ago by mcs

See #17917 for a follow up ticket.

comment:17 Changed 9 months ago by arthuredelstein

Keywords: tbb-no-uplift added
Note: See TracTickets for help on using tickets.