Opened 2 years ago

Closed 22 months ago

Last modified 22 months ago

#19273 closed task (fixed)

Write C++ patch to replace external applications helper dialog workaround

Reported by: gk Owned by: mcs
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201607R, tbb-usability, tbb-torbutton-conversion
Cc: mcs, brade, arthuredelstein, bugzilla Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorU

Description

Part of our deliverable should be to get rid of out clumsy attempts to hook the external applications helper dialog. We should patch the Mozilla code instead doing trying to fix that from extensions land. This might avoid #18090 and other weird side-effects.

Child Tickets

Change History (13)

comment:1 Changed 2 years ago by mcs

Owner: changed from tbb-team to mcs
Status: newassigned

comment:2 Changed 23 months ago by mcs

Cc: arthuredelstein added
Keywords: TorBrowserTeam201606R added; TorBrowserTeam201606 removed
Status: assignedneeds_review

Here is a tor-browser patch that generates "external-app-requested" observer service notifications that Torbutton can use to prompt and optionally cancel the launch of an external application:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug19273-01&id=0dff6c74126b5b52d4d7504644365af30985042b

and here is a Torbutton patch that uses the new notification (and removes a lot of old JS code):
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19273-01&id=d9ce9236ab4e2d371426e9e25d16e804ed5b2332

Please review.

comment:3 Changed 23 months ago by cypherpunks

It looks like you will get a random result, if there are multiple observers that disagree. Presumably, there is only one right now, but what happens if this code is upstreamed?

comment:4 in reply to:  3 Changed 23 months ago by mcs

Replying to cypherpunks:

It looks like you will get a random result, if there are multiple observers that disagree. Presumably, there is only one right now, but what happens if this code is upstreamed?

Good point. The observer service does not provide a way to avoid notifying all the observers, but we could specify that all modules that handle "external-app-requested" should not do anything if cancel has already been requested (subject.data == true). I think that will solve the problem.

comment:6 Changed 23 months ago by gk

Keywords: TorBrowserTeam201607R added; TorBrowserTeam201606R removed

comment:7 Changed 23 months ago by arthuredelstein

I have looked over the code and I think it looks good. (I am still building it and will report if I see any problems.)

General question: do you think a fix for https://bugzilla.mozilla.org/show_bug.cgi?id=440892 would use this observer mechanism? Or could a proper check for "network.protocol-handler.warn-external" be simply built directly into the code? I'm wondering what the right approach would be for future upstreaming.

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

Replying to arthuredelstein:

I have looked over the code and I think it looks good. (I am still building it and will report if I see any problems.)

Thanks for looking at the code.

General question: do you think a fix for https://bugzilla.mozilla.org/show_bug.cgi?id=440892 would use this observer mechanism? Or could a proper check for "network.protocol-handler.warn-external" be simply built directly into the code? I'm wondering what the right approach would be for future upstreaming.

I think a proper fix should just be built into the code (probably inside uriloader/exthandler/nsHandlerService.js). But given the uncertainty that was expressed by mikeperry in the following comment, Kathy and I thought it would be best to mimic the existing hooks that were in Torbutton (replacing the component overrides with an observer notification):
https://trac.torproject.org/projects/tor/ticket/9901#comment:74

Also, if we want to include a custom message it is easier to have our own separate dialog (although we did experiment with using a XUL overlay; see ticket:16623#comment:3).

comment:9 Changed 22 months ago by gk

Keywords: tbb-usability added
Resolution: fixed
Status: needs_reviewclosed

Okay, finally. Looks good to me and I am happy to see my crazy workaround going. I made a fixup commit removing the remaining bits from my #9901 patch we don't need anymore (7dd710152955bd4145814c8bafed3b50dfacff75 on torbutton master). mcs' and brade's patches are commit 6d5dc7845579ca99d9c46d921876e477ba60935f on torbutton master and commit 4b7123f235465f26052ea0dfaeebb590a3f31e0f on tor-browser-45.2.0esr-6.5-1.

This will be in the next alpha for broader testing. I expect this will help with usability things like #18090 as well.

comment:10 Changed 22 months ago by gk

Cc: bugzilla added

#18090 is a duplicate of this one.

comment:11 Changed 22 months ago by gk

Keywords: tbb-torbutton-conversion added

comment:12 in reply to:  9 Changed 22 months ago by brade

Replying to gk:

Okay, finally. Looks good to me and I am happy to see my crazy workaround going. I made a fixup commit removing the remaining bits from my #9901 patch we don't need anymore (7dd710152955bd4145814c8bafed3b50dfacff75 on torbutton master). mcs' and brade's patches are commit 6d5dc7845579ca99d9c46d921876e477ba60935f on torbutton master and commit 4b7123f235465f26052ea0dfaeebb590a3f31e0f on tor-browser-45.2.0esr-6.5-1.

Thanks for your review and for catching what we missed.

comment:13 Changed 22 months ago by bugzilla

#18778, #18090, #18554 should be child tickets. Also regression tests are highly needed for this ticket.

Last edited 22 months ago by bugzilla (previous) (diff)
Note: See TracTickets for help on using tickets.