See bug 1200869 as an example of the problem. These APIs don't touch the out string argument if the header doesn't exist, and instead throw an exception. This makes many C++ callers potentially unsafe since they may end up using whatever was in the string before the call. Jason, do you think we can/should fix this?
I can't see 1180091 (cc me?) This behavior is exposed to JS currently, so I'm guessing we should be careful about breaking addsons. Would you be happier with an alternate API that writes a blank string into the argument? (Note that this then makes it ambiguous whether a header was present and blank-valued, or missing entirely).
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #1) > I can't see 1180091 (cc me?) Looks like someone beat me to it! :-) > This behavior is exposed to JS currently, so I'm guessing we should be > careful about breaking addsons. Is it though? nsHttpHeaderArray returns NS_ERROR_NOT_AVAILABLE which will propagate up to these functions, and then XPConnect converts that error into an exception, so JS never gets to see the return value. It is only internal C++ callers that we need to care about. My question was mostly regarding those callers.
I'm testing a patch on try to see what happens!
Created attachment 8656550 [details] [diff] [review] Return an empty string for a header when an error occurs This fixes nsIHttpChannel::GetRequestHeader() and nsIHttpChannel::GetResponseHeader() to always empty out their string argument even when they fail. This prevents programming mistakes of passing the same string object to multiple of these calls and using the string value without checking the nsresult error code, since otherwise the string value may be unchanged from a previous call. Note that this doesn't affect JS consumers of these APIs since we only empty out the string argument in case the method fails, which will be translated to a JS exception, and the JS code will never get to see the emptied string.
Hmm, Liz, why do you think this should be tracked for 43? I don't think this is a serious issue, it's more about eliminating a footgun in our code for the future.
Ehsan: I was following up from another bug and thought this sounded like something that people may want to uplift (if it gets fixed). I'll just untrack it since you think it's not important for relman to follow up. Thanks for the feedback!
I don't think we'd want to uplift this. Thanks for checking!
Comment on attachment 8656550 [details] [diff] [review] Return an empty string for a header when an error occurs Review of attachment 8656550 [details] [diff] [review]: ----------------------------------------------------------------- LGTM Jason asked me to review this bug. I thing that his comment in this bug was actually meant for bug 815299.
(In reply to Dragana Damjanovic [:dragana] from comment #9) > Jason asked me to review this bug. I thing that his comment in this bug was > actually meant for bug 815299. Hmm, no, why do you think that?
(In reply to Ehsan Akhgari (don't ask for review please) from comment #10) > (In reply to Dragana Damjanovic [:dragana] from comment #9) > > Jason asked me to review this bug. I thing that his comment in this bug was > > actually meant for bug 815299. > > Hmm, no, why do you think that? I am probably wrong. My thoughts: Because actually being able to write empty string into request header could break addons if they always expect to get a value if the call to GetRequestHeader does not throw. Also both patches independently could potentially break addons, hopefully they will not.
Oh, I see what you mean now. Yes, that bug can break add-ons at least in theory, but this one cannot (please see comment 2.)