1148682 - Handle content length correctly for moz-icon channels

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Right now moz-icon channels return a bogus content length. At least one implementation returns uninitialized memory despite yielding a status of NS_OK.

We should either return the correct content length, or just signal failure.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Handle content length correctly for moz-icon channels.

OK, it doesn't seem trivial to get the correct content length here, and it's not
really very important in the end for these tiny icons, so on the platforms where
GetContentLength is not just forwarded to an underlying channel, I've taken the
approach of removing mContentLength and returning NS_ERROR_FAILURE.

On the platforms where GetContentLength *is* just forwarded to an underlying
channel, we don't need to do anything. That's why this patch only touches the OS
X and Windows implementations.

Try job for the two affected platforms here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1918d69b2f1c

Comment 2

[Seth Fowler [:seth] [:s2h]]

I've verified that this fixes the issue in bug 1145762.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8584923 [details] [diff] [review]
Handle content length correctly for moz-icon channels.

Hmm, moz-git-tools does not seem to be setting the review flag for me...

Comment 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8584923 [details] [diff] [review]
Handle content length correctly for moz-icon channels.

Do we need to implement GetContentLength at all? Can we just delete them and let whatever the parent class does?

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #4)
> Comment on attachment 8584923 [details] [diff] [review]
> Handle content length correctly for moz-icon channels.
> 
> Do we need to implement GetContentLength at all? Can we just delete them and
> let whatever the parent class does?

I think we do for these platforms. The other platforms forward to an underlying channel, but on OS X and Windows we don't have that option.

Comment 6

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d935ebcbfc63

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d935ebcbfc63

Comment 8

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8584923 [details] [diff] [review]
Handle content length correctly for moz-icon channels.

Approval Request Comment
[Feature/regressing bug #]: Truly ancient. This fixes bug 1145762.
[User impact if declined]: Unnecessary memory allocation, resulting in some cases in OOM.
[Describe test coverage new/current, TreeHerder]: On m-c (and I think now m-a).
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8584923 [details] [diff] [review]
Handle content length correctly for moz-icon channels.

Should be in 38 beta 2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5f5a4c5a7e02