Move open2 and asyncOpen2 to being the only implementaion
Categories
(Core :: DOM: Core & HTML, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox67 | --- | fixed |
People
(Reporter: jkt, Assigned: jkt)
References
Details
Attachments
(1 file)
AsyncOpen and Open is no longer used and we should move the 2 variants to being the only way of opening a channel.
Assignee | ||
Comment 1•5 years ago
|
||
Replacing js and text occurences of asyncOpen2
Replacing open2 with open
Assignee | ||
Comment 2•5 years ago
|
||
Comment 4•5 years ago
|
||
This is likely to break Thunderbird because c-c is still using the aListener
argument of AsyncOpen
.
Comment 5•5 years ago
|
||
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.
Assignee | ||
Comment 6•5 years ago
|
||
: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•5 years ago
•
|
||
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:
-
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).
-
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.
-
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•5 years ago
|
||
OK, I'll do the digging in bug 1452600. There's already an incomplete patch there that ignores the context issue.
Comment 9•5 years ago
|
||
Any ETA for landing this? It's a rather large patch that could fail to apply soon.
Assignee | ||
Comment 10•5 years ago
|
||
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•5 years ago
|
||
Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/74f99033251c Replacing AsyncOpen2 with AsyncOpen always r=valentin
![]() |
||
Comment 12•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Description
•