458158 - Crash [@ nsJAR::Open] when passing null argument to open method of zip-reader

Status: VERIFIED FIXED

(Core :: Networking: JAR, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase (uses enhanced privileges)

See testcase, which crashes when passing null to the zip-reader when using the open method.
http://crash-stats.mozilla.com/report/pending/01ec3442-9031-11dd-9367-001cc45a2ce4

Comment 1

[Martijn Wargers (dead)]

Btw, this was filed as a spin-off from bug 458108.

0  	xul.dll  	nsJAR::Open  	 modules/libjar/nsJAR.cpp:174
1 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
2 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2405

Comment 2

[IU]

I get a blank page if I navigate to that testcase.  Is something missing?

Comment 3

[Martijn Wargers (dead)]

The testcase uses enhanced privileges, you need to download it to your computer, so the enhanced privileges prompt comes up.

Comment 4

[Martijn Wargers (dead)]

Attachment: patch

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 341425 [details] [diff] [review]
patch

please also add a unit test

Comment 6

[-fullmetaljacket-]

checkin needed?

this crashing my browser every time i download a file.

http://crash-stats.mozilla.com/report/index/176b39e8-9017-11dd-80cf-001a4bd43ef6

Comment 7

[Martijn Wargers (dead)]

Well, I need to add a unit test first.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Using 3.1b1, I'm seeing the same behavior as comment 6: a crash at the end of every single file download.  For the moment, I've reverted to 3.0.

Comment 10

[Iain Curtain]

I'm experiencing the same issue:
http://crash-stats.mozilla.com/report/index/04be8a54-96b4-11dd-99b7-001cc45a2ce4

Comment 11

[Iain Curtain]

I'm still seeing this issue on the latest Mac nightly build.

Any chance of getting fixed soon?

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081202 Shiretoko/3.1b3pre

Comment 12

[timeless]

sooner if you write a unit test

we don't care about crashes anymore, only unit tests.

Comment 13

[Jeff Walden [:Waldo]]

C'mon, that's not accurate.  The reality is that writing a test for this is an exercise in triviality, and it's not unreasonable to say that exercise must be completed here.  It's not like this crash is one you're going to hit in normal use -- passing null is already a bug, so things aren't necessarily going to work in the calling code anyway, even if crashing is a bit severe for a reaction to an API violation.

But really, if you'd like to see this fixed, it should be pretty easy to create a test patterned off of <http://mxr.mozilla.org/mozilla-central/source/modules/libjar/test/unit/test_bug333423.js> that could be used here.

Comment 14

[Martijn Wargers (dead)]

Sorry for not providing a unit test, but I first need to learn how that stuff works. I'll try to come up with a unit test within the next week or so.

Comment 15

[(not currently active) Ted Mielczarek]

(In reply to comment #9)
> Using 3.1b1, I'm seeing the same behavior as comment 6: a crash at the end of
> every single file download.  For the moment, I've reverted to 3.0.

I'm now seeing this with trunk builds. I think this means that there's a broken caller out there, and this patch just wallpapers the problem. Granted, it might be the right color wallpaper...

Comment 16

[(not currently active) Ted Mielczarek]

Aha, turns out it was the Prism extension that was crashing me. I disabled extension compat checking yesterday after updating to latest trunk. Removing that extension stops this crash on file downloads. -fullmetaljacket-, dmose, if you have that extension you might want to disable it.

Comment 17

[Iain Curtain]

Confirm that it's Prism (0.2.1) causing the crash as well.

Comment 18

[IU]

This bug was filed in response to bug 458108.  I'll add a dependency.

Comment 19

[IU]

Could someone also please confirm bug 458108.  It has long been left as unconfirmed.

Thanks

Comment 20

[-fullmetaljacket-]

(In reply to comment #16)

yes it was prism (bug 458108) and older version of weave used to crash ff, if i remember it right, but fixed by update to weave 0.2.7.

Comment 21

[Benjamin Smedberg]

Anyone know why this hasn't landed yet? I don't think this is a blocker, but we should take it once it bakes on trunk.

Comment 22

[Jeff Walden [:Waldo]]

It needs a test, and it's easy to write one.  It's not like this is tricky functionality that's difficult to test due to race conditions or codebase inflexibility, it's just that no one's gone through the effort to write it yet.

Comment 23

[Martijn Wargers (dead)]

Sorry for taking so long to write a test, I'll to come up with something this week, hopefully.

Comment 24

[IU]

At this rate, I don't think this is going to make 3.1. :-(

Comment 25

[Martijn Wargers (dead)]

Bug 458108 is fixed, so this shouldn't be a real issue anymore.

Comment 26

[Jeff Walden [:Waldo]]

Martijn, if you need help writing this, ping me on IRC any time tomorrow -- it's less than five minutes tops, I think.

Comment 27

[Martijn Wargers (dead)]

Attachment: patch2

Sorry for the delay!

Ok, now I've added the xpcshell unit test. Without the patch, the xpcshell test causes a crash in the xpcshell.exe, when running make check in "C:\mozilla-build-1.3\src\_firefox\modules\libjar". After I applied the patch the xpchell test passes.

Comment 28

[Jeff Walden [:Waldo]]

Comment on attachment 358013 [details] [diff] [review]
patch2

>diff --git a/modules/libjar/test/unit/test_bug458158.js b/modules/libjar/test/unit/test_bug458158.js

>+  } catch (e if ((e instanceof Components.interfaces.nsIException) &&
>+                 (e.result == Components.results.NS_ERROR_NULL_POINTER))) {
>+    // do nothing, this test passes
>+  }

Please remove the redundant parentheses on the two conditions.

Comment 29

[Martijn Wargers (dead)]

Attachment: patch2 with the redundant parentheses removed

Btw, I copied that code from another unit test, which still has these redundant parentheses.

Comment 30

[Martijn Wargers (dead)]

Checked in:
$ hg tip
changeset:   24060:b398934d235f
tag:         tip
user:        Martijn Wargers <[e-mail]>
date:        Thu Jan 22 16:12:00 2009 +0100
summary:     Bug 458158 - Crash [@ nsJAR::Open] when passing null argument to op
en method of zip-reader, r+sr=cbiesinger and r=jwalden+bmo

Comment 31

[Jeff Walden [:Waldo]]

Thanks for writing the test!

Comment 32

[Samuel Sidler (old account; do not CC)]

The current #8 crash on the 1.9.1 branch (Firefox 3.1b3pre) crashes in this stack. We should really try to get this in for beta 3 to see if it's fixed.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks like it only has to be checked into 1.9.1 branch? Or do I miss something?

Comment 34

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset da28c92f6c2c to mozilla-1.9.1.

Comment 35

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed on trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090423 Minefield/3.6a1pre ID:20090423040020