1637826 - Port bug 1196151 - add second argument to nsIExternalProtocolService.loadURI calls

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2bd6adda1976
Port bug 1196151 - Add second argument when making nsIExternalProtocolService.loadURI call. rs=bustage-fix

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1637826-loaduri-second-arg.diff

I've added null as a second argument to all calls that didn't already have one. I assume that's acceptable, as it works enough to fix the tests, but maybe we should be using a real second argument in some or all of the places.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9148217 [details] [diff] [review]
1637826-loaduri-second-arg.diff

Review of attachment 9148217 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok, I didn't spot any place where passing a browsercontext would do anything.

Comment 4

[Richard Marti (:Paenglab)]

It seems something is missing. Clicking on a link like a bug number in a BZ bugmail does nothing but I get following error:

Uncaught contentAreaClick.js:230
Exception { name: "NS_ERROR_ILLEGAL_VALUE", message: "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIExternalProtocolService.loadURI]", result: 2147942487, filename: "chrome://communicator/content/contentAreaClick.js", lineNumber: 230, columnNumber: 0, data: null, stack: "openLinkExternally@chrome://communicator/content/contentAreaClick.js:230:6\ncontentAreaClick@chrome://communicator/content/contentAreaClick.js:200:21\nonclick@chrome://messenger/content/messenger.xhtml:1:8\n", location: XPCWrappedNative_NoHelper }
openLinkExternally chrome://communicator/content/contentAreaClick.js:230
contentAreaClick chrome://communicator/content/contentAreaClick.js:200
onclick chrome://messenger/content/messenger.xhtml:1

​

Comment 5

[Magnus Melin [:mkmelin]]

Ah, null browsingContext isn't allowed - https://hg.mozilla.org/mozilla-central/diff/d6c5410fc2859ce35774009947841ce7a68f1711/uriloader/exthandler/nsExternalHelperAppService.cpp#l1.15

Can we allow that when we just want it to open externally?

Comment 6

[:Gijs (he/him)]

(In reply to Magnus Melin [:mkmelin] from comment #5)

Ah, null browsingContext isn't allowed - https://hg.mozilla.org/mozilla-central/diff/d6c5410fc2859ce35774009947841ce7a68f1711/uriloader/exthandler/nsExternalHelperAppService.cpp#l1.15

Can we allow that when we just want it to open externally?

I'm not convinced we want to. The consumer doesn't normally know whether the URI will be opened externally, that's why it's calling the external helper app service. It's explicitly delegating how to open the URI to the app service. If it specifically wanted to open things externally, it could invoke the relevant helper app with the URI manually (via launchWithURI), right? I guess if TB knows that there are never any web protocol handlers, that's sort of an edgecase that wasn't considered, because Firefox can never know that for sure...

On the flip side, I had to leave the argument optional for launchWithURI on the mime info objects because the mimetype (not URI) case doesn't always have access to a browsing context - but that's more to do with the fact that they run downloads that aren't supposed to be associated to a particular content window.

Do all the consumers here have a content area from where the links they want to open originate? It's not clear to me from looking at the patch, given the lack of context...

The point of having the browser context is that if we want to load in the same context where the URL came from, or need to pop up a dialog a la unknownContentType, we have something to work with in terms of parenting that dialog. It's also likely we'll add permission support for external protocols soon (so webpages can't just force-open, uh, say, the Zoom video calling app...) and there too, we're likely to want to use browsing contexts to work out where to ask for permission and/or if we already have permission (I'm not sure off-hand if we'll need to do that behind this entrypoint or will do it elsewhere).

So I'm hesitant about relaxing the requirement. If we did, we should continue to enforce that the browsing context param is passed when invoked in the child process.

Matt, what do you think?

Comment 7

[Geoff Lankow (:darktrojan)]

(In reply to :Gijs (he/him) from comment #6)

Do all the consumers here have a content area from where the links they want to open originate? It's not clear to me from looking at the patch, given the lack of context...

Some of them are in content areas, but a lot of them are links in the UI itself.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to :Gijs (he/him) from comment #6)

So I'm hesitant about relaxing the requirement. If we did, we should continue to enforce that the browsing context param is passed when invoked in the child process.

Matt, what do you think?

I can't say I've looked at this code a whole lot, but can we just not require a browsing context until we know we're handing off to something that will use it?

I also think we should look towards blocking access to the external helper app entirely from the content process. DocumentChannel should be making us only access it from the parent now, unless I've missed a use case.

Comment 9

[:Gijs (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)

(In reply to :Gijs (he/him) from comment #6)

So I'm hesitant about relaxing the requirement. If we did, we should continue to enforce that the browsing context param is passed when invoked in the child process.

Matt, what do you think?

I can't say I've looked at this code a whole lot, but can we just not require a browsing context until we know we're handing off to something that will use it?

Yeah - today someone also pointed out that I broke "Email link", and then I doublechecked and there are Firefox/Toolkit consumers that do the same thing (though a bunch look like they're unreachable, and none appear to have tests, or we would have caught this when landing...).

I morphed bug 1638092 to un-require it, which will fix all these cases.

I also think we should look towards blocking access to the external helper app entirely from the content process. DocumentChannel should be making us only access it from the parent now, unless I've missed a use case.

I don't know, but I filed bug 1638269 for this.

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a2eeb5818b3a
Backed out changeset 2bd6adda1976 .

Comment 13

[Magnus Melin [:mkmelin]]

With bug 1638092, I think we're done here.