If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsIZipWriter crashes FF3 on access to nsIZipWriter.file [@ nsZipWriter::GetFile]

VERIFIED FIXED in mozilla1.9.1a1

Status

()

Core
General
--
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: John J. Barton, Assigned: mossop)

Tracking

({crash, verified1.9.0.1})

Trunk
mozilla1.9.1a1
crash, verified1.9.0.1
Points:
---
Bug Flags:
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
http://crash-stats.mozilla.com/report/index/04f0d62e-1f79-11dd-a944-0013211cbf8a

------------
        var zipWriter = Components.Constructor("@mozilla.org/zipwriter;1", "nsIZipWriter");
        var writer = new zipWriter();

        dump("Trying to crash\n");
        dump("nsIZipWriter.file:"+writer.file+"\n");
        dump("Failed to crash\n");
-------------
Note there is no call to open(). I was on a breakpoint in chromebug and try to show the properties of 'writer'. Traversing the property list crashed at "file".
(Assignee)

Comment 1

10 years ago
Silly mistake. Not a blocker I think but we could probably get it in a maintenance release.
Component: General → General
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk

Updated

10 years ago
Severity: normal → critical
Keywords: crash
Summary: nsIZipWriter crashes FF3 on access to nsIZipWriter.file → nsIZipWriter crashes FF3 on access to nsIZipWriter.file [@ nsZipWriter::GetFile]
(Assignee)

Comment 2

10 years ago
Created attachment 320522 [details] [diff] [review]
patch rev 1

Really simple fix, just null checks and throws out an error if the zipwriter isn't initialised yet. Also includes a unit test that checks most of the properties and methods for this.
Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Attachment #320522 - Flags: review?(jst)

Updated

10 years ago
Attachment #320522 - Flags: review?(jst) → review+
(Assignee)

Comment 3

10 years ago
Comment on attachment 320522 [details] [diff] [review]
patch rev 1

This crasher will be rare however the fix for it is trivial and risk free and comes complete with a unit test. Would be nice to land in the event of an RC2 or if not then for a maintenance release
Attachment #320522 - Flags: approval1.9?
(Assignee)

Comment 4

9 years ago
Landed in changeset 79967bfbed1b
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Flags: blocking1.9.0.1?

Comment 5

9 years ago
Is there any way to verify this? Some sort of testcase?
(Assignee)

Comment 6

9 years ago
(In reply to comment #5)
> Is there any way to verify this? Some sort of testcase?

It wouldn't be impossible to write an extension that tests it, but the patch includes an automated test to verify the fix so I don't believe it is necessary
(Assignee)

Updated

9 years ago
Attachment #320522 - Flags: approval1.9? → approval1.9.0.1?
Comment on attachment 320522 [details] [diff] [review]
patch rev 1

>diff --git a/modules/libjar/zipwriter/test/unit/test_bug433248.js b/modules/libjar/zipwriter/test/unit/test_bug433248.js
...
>+function run_test()
>+{
>+  var test;
>+  // zipW is an uninitialised zipwriter at this point.
>+  try {
>+    test = zipW.file;
>+    do_throw("Should have thrown unitialised error.");
>+  }
>+  catch (e) {
>+    do_check_eq(e.result, Components.results.NS_ERROR_NOT_INITIALIZED);
>+  }

Two observations (sorry it's after the fact).  First, please fix the spelling error in do_throw() lines throughout this patch (you really don't want to be on bug 106386).

Second, I've seen a lot of xpcshell tests over the last several months where we are looking for a specific exception code.  I think we should standardize that into a single do_check_exception() in head.js sometime soon.
Comment on attachment 320522 [details] [diff] [review]
patch rev 1

a=beltzner
Attachment #320522 - Flags: approval1.9.0.1? → approval1.9.0.1+
Reed will get this before tonight's freeze.
Keywords: checkin-needed
Flags: blocking1.9.0.1? → wanted1.9.0.x+
mozilla/modules/libjar/zipwriter/src/nsZipWriter.cpp 	1.3
mozilla/modules/libjar/zipwriter/test/unit/test_bug433248.js 	1.1 
Keywords: checkin-needed → fixed1.9.0.1
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1a1
Fixed the spelling mistake in Hg and CVS:
mozilla/modules/libjar/zipwriter/test/unit/test_bug433248.js 	1.2 
http://hg.mozilla.org/mozilla-central/index.cgi/rev/82a38df7573f
going to mark this verified for Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1) Gecko/2008070206 Firefox/3.0.1, based on the patch in comment 11.  going to trust that the unit tests are passing green.  Please reopen if otherwise.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.1 → verified1.9.0.1
Crash Signature: [@ nsZipWriter::GetFile]
You need to log in before you can comment on or make changes to this bug.