1528571 - Missing error handling when calling AddHeadersToChannel() inside nsDocShell::DoURILoad()

Status: NEW

(Core :: DOM: Navigation, enhancement, P3)

Description

[(no longer active)]

Coverity points to this write-only rv variable https://searchfox.org/mozilla-central/rev/e6520f0a4dd5d7474c32a1164744953ea59be0d0/docshell/base/nsDocShell.cpp#10140 which indicates we're forgetting to handle the error return values from AddHeadersToChannel() (which indeed seems to be fallible.)

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Interesting. That return value is definitely not ever used. This doesn't appear to be a recent change.

From my quick look over this, it appears that this method can fail in a couple of ways:

1. The nsIInputStream to produce the header stream data fails
2. The header stream data is malformed, or
3. Setting a header on the nsIHttpChannel fails.

However, there is actually only a single codepath which can cause a headers stream to be set on a navigation like this, all other calls either use nullptr, or are simply forwarding the value around. That one codepath is here: https://searchfox.org/mozilla-central/rev/ddb81c7a43ffada1f6cb4200c4f625e50e44dcf3/dom/plugins/base/nsPluginInstanceOwner.cpp#450-453

The nsIInputStream doesn't actually need to be one here, as the value being passed in is always a string input stream, which was derived from a const char* value passed to us by an NPAPI plugin.

Looking at the callsites of that method, it looks even weirder - tracing stuff back even further, and trimming off branches which just pass in nullptr, it looks like https://searchfox.org/mozilla-central/rev/ddb81c7a43ffada1f6cb4200c4f625e50e44dcf3/dom/plugins/base/nsPluginHost.cpp#639-640 is the only callsite which would pass a value in, though its only callsite also passes in nullptr for the headers parameter: https://searchfox.org/mozilla-central/rev/ddb81c7a43ffada1f6cb4200c4f625e50e44dcf3/dom/plugins/base/nsNPAPIPlugin.cpp#368

In summary, I think that it is currently impossible to set a HeadersChannel on an nsDocShell load, and that the feature only existed for legacy NPAPI features which have since been removed, so this isn't an issue, but rather a sign of dead code.

ni? :bz for thoughts and potentially more context.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Looks like the header bits have basically been dead code for a while now?

My temptation would be to add telemetry to make sure we're not missing something stupid involving flash here, and if that turns out to show that we never have a non-null headers stream, we should remove the functionality.

I guess we should also add a check that at https://searchfox.org/mozilla-central/rev/ddb81c7a43ffada1f6cb4200c4f625e50e44dcf3/docshell/base/nsDocShell.cpp#4005 mHeaders is null and make sure that's green in at least automation? That said, https://codecov.io/gh/mozilla/gecko-dev/src/766636073f59212b3b93bf1f0fe814b3dec51623/docshell/base/nsDocShell.cpp claims nsDocShell::AddHeadersToChannel is NOT dead code, so I assume we end up using this somewhere. Possibly only in tests?

Comment 3

[Boris Zbarsky [:bzbarsky]]

And of course we do use this in tests. The question is whether this code is test-only.

We use this at https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/toolkit/mozapps/extensions/content/extensions.js#2117-2126 (getClientHeader()) which then gets called by callsites of https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/toolkit/mozapps/extensions/content/extensions.js#2255 (_loadURL) which does a load in a docshell and passes these headers. Looks like that was added in bug 1489531?

If we do keep this functionality, we should just take a string instead of taking a stream that all callers have to produce from a string, just so we can get a string from it again.

Comment 4

[Andrew Swan [:aswan]]

The code linked from comment 3 is not test-only (or in other words, it is invoked in normal browser usage when users are looking at the "Get Add-ons" pane of about:addons).
However, the "Get Add-ons" page is going to move from a remotely loaded web page to something implemented within the browser, so that code can/will be removed relatively soon (I would guess during the Firefox 70 cycle).
Bug 1540173 if you would like to track that project.