Handle content length correctly for moz-icon channels

RESOLVED FIXED in Firefox 38

Status

()

Firefox
Downloads Panel
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
Firefox 39
Points:
---

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 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

2 years ago
Created attachment 8584923 [details] [diff] [review]
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
(Assignee)

Comment 2

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

Comment 3

2 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

2 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+
(Assignee)

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d935ebcbfc63
https://hg.mozilla.org/mozilla-central/rev/d935ebcbfc63
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
(Assignee)

Comment 8

2 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?
status-firefox38: --- → affected
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+
https://hg.mozilla.org/releases/mozilla-beta/rev/5f5a4c5a7e02
status-firefox38: affected → fixed
(Assignee)

Updated

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