1718324 - Move MozIcon to something closer to ExtensionProtocolHandler

Status: NEW

(Core :: Widget: Gtk, defect)

Description

[:gerard-majax]

https://phabricator.services.mozilla.com/D118596#3865440:

I think that this channel won't behave very well if opened with Open, but I don't think it'll lead to a deadlock due to some quirks in how necko handles channels. I think this code will actually end up with an empty payload in that situation.

The problem here is that this code violates the requirements of the Open method on nsIChannel by returning a non-blocking input stream in that situation (https://searchfox.org/mozilla-central/rev/fc95c6ad297d9d257f05599d01741503f3f57326/netwerk/base/nsIChannel.idl#144). This happens because the nsInputStreamChannel class always produces the same nsIInputStream which was passed in from OpenContentStream whether or not async is true (https://searchfox.org/mozilla-central/rev/fc95c6ad297d9d257f05599d01741503f3f57326/netwerk/base/nsInputStreamChannel.cpp#14-38). Code which is expecting to get a blocking input stream from Open will then be given this async input stream, and will attempt to read from it in a blocking manner, which I think should cause no data to be read and for the function to return instantly (IIRC there's no way to do a truly blocking read on a non-blocking nsPipeInputStream). If we were to instead return NS_ERROR_NOT_IMPLEMENTED from that OpenContentStream function our caller in Open would actually spin a nested event loop waiting for data to become available after calling AsyncOpen which also wouldn't technically block (though it would be a nested event loop :-/)

Based on the ccov information for the windows nsIconChannel it appears that we don't actually try to do a sync Open of moz-icon channels anyway (https://searchfox.org/mozilla-central/rev/fc95c6ad297d9d257f05599d01741503f3f57326/image/decoders/icon/win/nsIconChannel.cpp#282-296), so this probably is a purely theoretical issue. If we weren't time-constrained here, I think I'd rework this to instead use NS_NewSimpleChannel and avoid most of these issues, but I think this is a pretty low-risk channel to modify, and I think it will be valuable to get experience with it being remoted earlier, even if we end up doing that change in a follow-up.

In conclusion, I think this channel will break when used in a sync manner, and I think that's probably OK for now. We should probably file a follow-up to improve on it in the future though, bringing the code more in-line with PageThumbProtocolHandler and ExtensionProtocolHandler, as I don't think we want to stick with this solution long-term.