Closed Bug 1637826 Opened 10 months ago Closed 10 months ago

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

Categories

(Thunderbird :: General, task)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 78.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(1 file)

No description provided.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/2bd6adda1976
Port bug 1196151 - Add second argument when making nsIExternalProtocolService.loadURI call. rs=bustage-fix

Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED

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.

Attachment #9148217 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9148217 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → Thunderbird 78.0

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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Regressions: 1637970

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?

Flags: needinfo?(gijskruitbosch+bugs)

(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?

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(gijskruitbosch+bugs)

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

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

Flags: needinfo?(matt.woodrow)

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

Depends on: 1638092
Flags: needinfo?(mkmelin+mozilla)
Keywords: leave-open
Backout by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/a2eeb5818b3a
Backed out changeset 2bd6adda1976 .
Duplicate of this bug: 1638075
No longer regressions: 1637970
Duplicate of this bug: 1637970

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

Status: REOPENED → RESOLVED
Closed: 10 months ago10 months ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.