Open Bug 1528571 Opened 11 months ago Updated 3 months ago

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


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





(Reporter: ehsan, Unassigned)


(Blocks 1 open bug)


Coverity points to this write-only rv variable which indicates we're forgetting to handle the error return values from AddHeadersToChannel() (which indeed seems to be fallible.)

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:

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 is the only callsite which would pass a value in, though its only callsite also passes in nullptr for the headers parameter:

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.

Flags: needinfo?(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 mHeaders is null and make sure that's green in at least automation? That said, claims nsDocShell::AddHeadersToChannel is NOT dead code, so I assume we end up using this somewhere. Possibly only in tests?

Flags: needinfo?(bzbarsky)

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

We use this at (getClientHeader()) which then gets called by callsites of (_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.

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.

Priority: -- → P3