The circuit display is not visible on error pages in Tor Browser based on ESR68

https://expired.badssl.com/ shows a circuit display in 9.0a4 but not in recent nightlies.

added TorBrowserTeam201909R actualpoints::0.5 component::applications/tor browser ff68-esr owner::tbb-team points::1 priority::medium resolution::fixed severity::normal status::closed tbb-9.0-must-alpha type::defect labels

Moving must-alpha tickets to September.

Trac:
Points: N/A to 1

Fix in https://github.com/acatarineu/torbutton/commit/31562. contentPrincipal.origin does not contain the original URI causing the error anymore, so switched to contentPrincipal.URI.spec.

Trac:
Status: new to needs_review

Trac:
Keywords: TorBrowserTeam201909 deleted, TorBrowserTeam201909R added

Are we sure browser.contentPrincipal.URI can't be null? It seems the Firefox folks think otherwise (see: https://searchfox.org/mozilla-esr68/source/browser/base/content/browser.js#1092 and the comment at https://searchfox.org/mozilla-esr60/source/browser/base/content/browser.js#800). We should adapt our code I think.

Are we sure origin was just syntactic sugar for URI.spec? We should be certain that we don't introduce a subtle bug here. I wonder what the new siteOrigin (https://bugzilla.mozilla.org/show_bug.cgi?id=1485177) would give us here. Maybe that's what we actually want?

Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201909R deleted, TorBrowserTeam201909 added

Thanks for the review.

It seems the Firefox folks think otherwise (see: ​https://searchfox.org/mozilla-esr68/source/browser/base/content/browser.js#1092 and the comment at ​https://searchfox.org/mozilla-esr60/source/browser/base/content/browser.js#800).

I'm not sure if the first check

if (firstPartyDomain === k_tb_about_uri_first_party_domain) {

would have ruled out the possibility of null URIs, but you're right, we should check that in general.

That code in the first link pointed me to a way to get the original URI that caused the error in (I think) a slightly less-hacky way: https://searchfox.org/mozilla-esr68/source/browser/base/content/browser-safebrowsing.js#10. It seems that when there is an error gBrowser.currentURI keeps the original URL, and gBrowser.selectedBrowser.documentURI (or the contentPrincipal) has the about:* one (gBrowser.currentURI is an alias of gBrowser.selectedBrowser.currentURI). This would have worked in esr60 too.

So here is a patch with this different approach: https://github.com/acatarineu/torbutton/commit/31562+1. I also removed the check with the hardcoded k_tb_about_uri_first_party_domain.

Trac:
Keywords: TorBrowserTeam201909 deleted, TorBrowserTeam201909R added
Status: needs_revision to needs_review

Looks almost good now, just a nit: please remove the trailing whitespace after

+  // Bug 31562: For neterror or certerror, get the original URL from

Trac:
Keywords: TorBrowserTeam201909R deleted, TorBrowserTeam201909 added
Status: needs_review to needs_revision

Done in https://github.com/acatarineu/torbutton/commit/31562+2.

Trac:
Keywords: TorBrowserTeam201909 deleted, TorBrowserTeam201909R added
Status: needs_revision to needs_review

Looks good. Cherry-picked to master (commit bb975009a65ab9039641d03b11023fe6c52f5b7a)

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

Trac:
Actualpoints: N/A to 0.5

closed

changed time estimate to 8h

added 4h of time spent

mentioned in issue #32297 (moved)

mentioned in issue #32297 (moved)

moved to tpo/applications/tor-browser#31562 (closed)