Handle content length correctly for moz-icon channels

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
Firefox 39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

4 years ago
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.
Assignee

Comment 1

4 years ago
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
Assignee

Comment 2

4 years ago
I've verified that this fixes the issue in bug 1145762.
Assignee

Comment 3

4 years ago
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...
Attachment #8584923 - Flags: review?(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?
Assignee

Comment 5

4 years ago
(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.
Attachment #8584923 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/d935ebcbfc63
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Assignee

Comment 8

4 years ago
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.
Attachment #8584923 - Flags: approval-mozilla-beta?
Comment on attachment 8584923 [details] [diff] [review]
Handle content length correctly for moz-icon channels.

Should be in 38 beta 2
Attachment #8584923 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee

Updated

4 years ago
Duplicate of this bug: 1152582
You need to log in before you can comment on or make changes to this bug.