1520868 - Move open2 and asyncOpen2 to being the only implementaion

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Jonathan Kingston [:jkt] he/him]

AsyncOpen and Open is no longer used and we should move the 2 variants to being the only way of opening a channel.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Attachment: Replacing AsyncOpen2 with AsyncOpen always

Replacing js and text occurences of asyncOpen2
Replacing open2 with open

Comment 2

[Jonathan Kingston [:jkt] he/him]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=230a841fed08217caee456f4fb5115aab87001fd

Comment 4

[Masatoshi Kimura [:emk]]

This is likely to break Thunderbird because c-c is still using the aListener argument of AsyncOpen.

Comment 5

[Jorg K (CEST = GMT+2)]

Yes, I know, that has been discussed in other bugs that intended to do the same thing. I was actually under the impression M-C dropped this project since bug 1411609 hadn't moved and Patrick asked in bug 1411609 comment #6:

(Patrick McManus [:mcmanus] from comment #6)

this is so fundamental - can you check with mailnews to see if it would make
their life difficult?

We'd have to go through all sorts of hoops to squeeze the aListener argument through the reduced interface, see bug 1411609 comment #20.

Comment 6

[Jonathan Kingston [:jkt] he/him]

:bz is there anything we can provide (even if temporary for thunderbird to ease this process)?

See your previous comment https://bugzilla.mozilla.org/show_bug.cgi?id=1411609#c20 and the attempt to implement this: https://bugzilla.mozilla.org/show_bug.cgi?id=1452600#c8

My argument against the context is that we are hand rolling these arguments and passing them around without verifying their state within Firefox which lends itself to security bugs.

Also having multiple implementations to AsyncOpen and Open is risky in the case where a class doesn't correctly extend it's parent (which can be seen in the FTP implementation where we don't implement AsyncOpen2 and inherit from the parent but it appears we do the right thing thankfully).
It's also a pretty difficult job to verify all the call sites and implementations of Channels to check GetEnforceSecurity isn't being set to false and skipping the steps.

I also started working on this as a mozilla employee was confused by the debug assertions they where hitting when calling Open instead of Open2 it's a confusing legacy that seems like it should be removed.

Another argument is it seems in addition to this patch we can remove mListenerContext and all the context arguments to OnProgress, OnStartRequest, OnStopRequest, AsyncConvertData etc as per https://bugzilla.mozilla.org/show_bug.cgi?id=1452600#c7

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, I don't quite understand the Thunderbird pushback here. I thought bug 1411609 comment 20 was pretty clear, if you know the set of channels involved.

Let's take a simple example, from the link in bug 1452600 comment 8, by way of illustration: the use in nsCopyMessageStreamListener::OnStartRequest. nsCopyMessageStreamListener is instantiated only via contract, looks like, and only in a few places: imap/src/nsImapMailFolder.cpp and local/src/nsLocalMailFolder.cpp. Both places get a URI for the message or folder being copied, then call CopyMessage/CopyMessages.

Now you have a couple of options:

1. You can dig through that stuff to see what context is passed to AsyncOpen() on the channel and just store it on the channel directly (via an interface you add, if these are channel types you control, or via the propertybag that pretty much every channel implements).

2. You can store the value directly in the nsCopyMessageStreamListener if you know it at the point when you create it. Again, might need digging down to the AsyncOpen call.

3. In this specific case, could you not get the URI from the request? It sure looks like the context is just the URI; is it different from the request's URI?

Now I agree this takes some work. But I agree that the current situation of having known-insecure AsyncOpen()/Open() methods in Gecko is not OK, and we should get rid of them. It's not like this is news; this has been on the agenda for literally years. If Thunderbird feels like they could use a few weeks to deal at this point, we can probably do that, but I wouldn't hold the change for a long time, honestly, much as it pains me to say that.

Comment 8

[Jorg K (CEST = GMT+2)]

OK, I'll do the digging in bug 1452600. There's already an incomplete patch there that ignores the context issue.

Comment 9

[Jorg K (CEST = GMT+2)]

Any ETA for landing this? It's a rather large patch that could fail to apply soon.

Comment 10

[Jonathan Kingston [:jkt] he/him]

I expect tomorrow as I broke something in a rebase and only just fixed that.

Also there is a cocoa compile error which I need to fix:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=99792728a88e51cea119b9ccae5fd30254df3a3a&selectedJob=226636246

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74f99033251c
Replacing AsyncOpen2 with AsyncOpen always r=valentin

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/74f99033251c