Last Comment Bug 684347 - nsZipWriter::addEntryChannel(queue=true) creates corrupt zip file
: nsZipWriter::addEntryChannel(queue=true) creates corrupt zip file
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Networking: JAR (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla10
Assigned To: Kris Maglione [:kmag]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-02 14:04 PDT by Kris Maglione [:kmag]
Modified: 2011-10-26 17:12 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (2.27 KB, text/plain)
2011-09-02 14:04 PDT, Kris Maglione [:kmag]
no flags Details
Proposed fix (2.27 KB, patch)
2011-09-02 14:05 PDT, Kris Maglione [:kmag]
no flags Details | Diff | Splinter Review
Updated patch (6.23 KB, patch)
2011-09-06 19:53 PDT, Kris Maglione [:kmag]
dtownsend: review+
Details | Diff | Splinter Review
Updated patch (6.23 KB, patch)
2011-10-25 15:59 PDT, Kris Maglione [:kmag]
no flags Details | Diff | Splinter Review
Updated commit message and authorship info. (6.36 KB, patch)
2011-10-25 16:38 PDT, Kris Maglione [:kmag]
no flags Details | Diff | Splinter Review

Description User image Kris Maglione [:kmag] 2011-09-02 14:04:11 PDT
Created attachment 557938 [details]
Proposed fix

When channels are processed from a queue, the header for an entry is initialized but never written out to the zip file, which leaves the file corrupt. The attached patch removes the code duplication for stream and channel processing and fixes the issue.
Comment 1 User image Kris Maglione [:kmag] 2011-09-02 14:05:26 PDT
Created attachment 557939 [details] [diff] [review]
Proposed fix
Comment 2 User image Dave Townsend [:mossop] 2011-09-06 09:47:10 PDT
Comment on attachment 557939 [details] [diff] [review]
Proposed fix

I'd like to see some unit tests for this before reviewing it
Comment 3 User image Kris Maglione [:kmag] 2011-09-06 10:09:03 PDT
Do you want a separate test for this bug, or should I add them to test_asyncadd.js where they already fit comfortably?
Comment 4 User image Kris Maglione [:kmag] 2011-09-06 19:53:20 PDT
Created attachment 558713 [details] [diff] [review]
Updated patch

Updated patch with tests.
Comment 5 User image Kris Maglione [:kmag] 2011-10-25 15:59:43 PDT
Created attachment 569533 [details] [diff] [review]
Updated patch

Updated after PRBool code churn. Can someone land this please?
Comment 6 User image Josh Matthews [:jdm] 2011-10-25 16:26:06 PDT
Actually Kris, if you could attach a patch that follows the conventions in http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed, it will be a lot easier to check in the patch.
Comment 7 User image Kris Maglione [:kmag] 2011-10-25 16:38:41 PDT
Created attachment 569543 [details] [diff] [review]
Updated commit message and authorship info.

jdm: Thanks.
Comment 9 User image Ed Morley [:emorley] 2011-10-26 17:12:10 PDT
https://hg.mozilla.org/mozilla-central/rev/926699a40a9b

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