Last Comment Bug 1148682 - Handle content length correctly for moz-icon channels
: Handle content length correctly for moz-icon channels
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Downloads Panel (show other bugs)
: unspecified
: All All
-- normal (vote)
: Firefox 39
Assigned To: Seth Fowler [:seth] [:s2h]
:
: :Paolo Amadini
Mentors:
: 1152582 (view as bug list)
Depends on:
Blocks: 1145762
  Show dependency treegraph
 
Reported: 2015-03-27 18:51 PDT by Seth Fowler [:seth] [:s2h]
Modified: 2015-04-08 16:33 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Handle content length correctly for moz-icon channels. (2.85 KB, patch)
2015-03-27 19:58 PDT, Seth Fowler [:seth] [:s2h]
tnikkel: review+
sledru: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Seth Fowler [:seth] [:s2h] 2015-03-27 18:51:32 PDT
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 User image Seth Fowler [:seth] [:s2h] 2015-03-27 19:58:39 PDT
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
Comment 2 User image Seth Fowler [:seth] [:s2h] 2015-03-27 20:53:36 PDT
I've verified that this fixes the issue in bug 1145762.
Comment 3 User image Seth Fowler [:seth] [:s2h] 2015-03-29 16:13:56 PDT
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 User image Timothy Nikkel (:tnikkel) 2015-03-29 16:22:24 PDT
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 User image Seth Fowler [:seth] [:s2h] 2015-03-29 16:24:54 PDT
(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 User image Seth Fowler [:seth] [:s2h] 2015-03-29 17:48:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d935ebcbfc63
Comment 7 User image Carsten Book [:Tomcat] 2015-03-30 03:10:14 PDT
https://hg.mozilla.org/mozilla-central/rev/d935ebcbfc63
Comment 8 User image Seth Fowler [:seth] [:s2h] 2015-03-30 14:29:20 PDT
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 User image Sylvestre Ledru [:sylvestre] 2015-04-02 06:18:09 PDT
Comment on attachment 8584923 [details] [diff] [review]
Handle content length correctly for moz-icon channels.

Should be in 38 beta 2
Comment 10 User image Ryan VanderMeulen [:RyanVM] 2015-04-03 12:25:01 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/5f5a4c5a7e02
Comment 11 User image Seth Fowler [:seth] [:s2h] 2015-04-08 16:33:53 PDT
*** Bug 1152582 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.