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

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 (moved) and other weird side-effects.

added TorBrowserTeam201607R component::applications/tor browser owner::mcs priority::high resolution::fixed severity::normal sponsor::U status::closed tbb-torbutton-conversion tbb-usability type::task labels

Trac:
Owner: tbb-team to mcs
Status: new to assigned

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.

Trac:
Cc: mcs, brade to mcs, brade, arthuredelstein
Status: assigned to needs_review
Keywords: TorBrowserTeam201606 deleted, TorBrowserTeam201606R added

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?

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.

Here is a revised Torbutton patch that follows the protocol I described in comment:4: https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19273-02&id=5e92e8c171ded5125aeeccfc2e6a4cfb974412ac

Trac:
Keywords: TorBrowserTeam201606R deleted, TorBrowserTeam201607R added

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.

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).

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 (closed) 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 (moved) as well.

Trac:
Resolution: N/A to fixed
Status: needs_review to closed
Keywords: N/A deleted, tbb-usability added

#18090 (moved) is a duplicate of this one.

Trac:
Cc: mcs, brade, arthuredelstein to mcs, brade, arthuredelstein, bugzilla

Trac:
Keywords: N/A deleted, tbb-torbutton-conversion added

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 (closed) 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.

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

closed

mentioned in issue #21766 (moved)

mentioned in issue #22618 (moved)

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