1521596 - protocol handler incognito fixups

Status: VERIFIED FIXED

(WebExtensions :: General, enhancement, P1)

Description

[Shane Caraveo (:mixedpuppy)]

protocol handlers wont work in private windows, however the ux needs work. Since it's not possible to load, marking at a lower priority.

There are two directions we can go.

1: let the extension message it

Currently a protocol handler page from an extension loads but without any access to any extension APIs. We could leave it at that, the extension could then recognize that APIs are not available, and show the user an error page with direction to enable the extension in private browsing.

2: handle it in external protocol service

We should check window access in nsExternalHelperAppService::LoadURI[1] and avoid loading. This would result in the app helper dialog appearing. The app helper dialog should only list helpers that can run in private windows.

[1] https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/uriloader/exthandler/nsExternalHelperAppService.cpp#914

Comment 1

[Shane Caraveo (:mixedpuppy)]

Thinking about this more, the whole protocol handling system would need to adjust for this. So I'd prefer the options of:

1: same as above, the page loads without API access (currently already does)
2: we do not enable the protocol_handler unless the extension has private browsing permission

mconca for input.

Comment 2

[Mike Conca [:mconca]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #1)

Thinking about this more, the whole protocol handling system would need to adjust for this. So I'd prefer the options of:

1: same as above, the page loads without API access (currently already does)
2: we do not enable the protocol_handler unless the extension has private browsing permission

mconca for input.

This sounds good to me. We should document this behavior on MDN under the protocol_handler section.

Comment 3

[Shane Caraveo (:mixedpuppy)]

I'm not sure which one you're saying is ok. #1 or #2?

Comment 4

[Mike Conca [:mconca]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)

I'm not sure which one you're saying is ok. #1 or #2?

...so tempted to reply with "yes"

Sorry. I think #2 follows the model for every other extension and makes the most sense.

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike Conca [:mconca] from comment #4)

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)

Sorry. I think #2 follows the model for every other extension and makes the most sense.

The general model is that it works in non-private, but not in private without permission. #2 is suggesting we do not enable it anywhere unless it has private permission.

Comment 6

[Mike Conca [:mconca]]

Shane and I talked this morning and it seems like we might be able to at least show the chooser dialog in PB windows, which would be a better user experience.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: protocol handler chooser

This is for text feedback. The label "Disabled in Private Browsing" will be shown when an extension handler is disallowed.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1521596 code cleanup and eslint

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1521596 add support for richlistitem.disabled

Depends on D19080

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1521596 incognito support for protocol handlers

Depends on D19081

Comment 11

[Meridel [:meridel]]

Comment on attachment 9042286 [details]
protocol handler chooser

I'm not too familiar with protocol handlers, unfortunately. Are we (Mozilla) doing the disabling for the user? I believe so. 

In that case, the verb "Disabled" works. I would adjust the string to be: "Disabled in Private Windows" (replacing "Private Browsing" with "Private Windows") to be consistent with other strings.

Comment 12

[Meridel [:meridel]]

Comment on attachment 9042286 [details]
protocol handler chooser

I'm not too familiar with protocol handlers, unfortunately. Are we (Mozilla) doing the disabling for the user? I believe so. 

In that case, the verb "Disabled" works. I would adjust the string to be: "Disabled in Private Windows" (replacing "Private Browsing" with "Private Windows") to be consistent with other UI strings.

Comment 13

[Meridel [:meridel]]

Comment on attachment 9042286 [details]
protocol handler chooser

I'm not too familiar with protocol handlers, unfortunately. Are we (Mozilla) doing the disabling for the user? I believe so. 

In that case, the verb "Disabled" works. I would adjust the string to be: "Disabled in Private Windows" (replacing "Private Browsing" with "Private Windows") to be consistent with other UI strings.

Comment 14

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d85d5970a4d5c61e2fcd97815b75fe6a91946fc

Comment 15

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b95374e21f5c48b370987235054502cdb7e66d70

Comment 16

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5fdcb9d550bd5ab2c76f8cc042c3091e3b41468

Comment 17

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebc2e45dd33201b5dd4e8ef67b5932695bd8effc

Comment 18

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12103a2ddb5b30f850553378d8edb71481016b7b

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1521596 expose private browsing flag in nsIRemoteWindowContext

Comment 20

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44dcaac98b0960c0a5d54fa067ef05f3dbd65c5e

Comment 21

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df3c623e4329e77625d66c34f481d8f5522eb234

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/575458230080
expose private browsing flag in nsIRemoteWindowContext r=kmag
https://hg.mozilla.org/integration/autoland/rev/91f7275e170c
code cleanup and eslint r=rpl
https://hg.mozilla.org/integration/autoland/rev/6d7343355004
add support for richlistitem.disabled r=bgrins,aswan
https://hg.mozilla.org/integration/autoland/rev/47bfb7727024
incognito support for protocol handlers r=flod,kmag

Comment 23

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/575458230080
https://hg.mozilla.org/mozilla-central/rev/91f7275e170c
https://hg.mozilla.org/mozilla-central/rev/6d7343355004
https://hg.mozilla.org/mozilla-central/rev/47bfb7727024

Comment 24

[Shane Caraveo (:mixedpuppy)]

Attachment: test-protocol.xpi

small extension for manual testing.

• install extension
• open tab to mailto:foobar (ok)
• open tab to ext+test:foobar (ok)
• open private window
• open tab to mailto:foobar (fail)
• open tab to ext+test:foobar (fail)

create an html page with links to the above urls

• in non-private window open html page
• click on links, both should open
• in private window open html page
• click on links, neither should open

Comment 25

[Shane Caraveo (:mixedpuppy)]

Further note: in cases where the url fails to open, you should get a dialog to choose an app to handle the link.

Comment 26

[CosminB]

Attachment: Bug1521596.gif

This issue is verified as fixed on Firefox 67.0a1 (20190306161300) under Win 7 64-bit and Mac OS X 10.14.1.

Please see the attached video.