Closed Bug 347752 Opened 14 years ago Closed 13 years ago

Crash [@ MSVCR80.dll] when using nsIProperties service get method with null argument


(Core :: XPCOM, defect, critical)

Windows XP
Not set





(Reporter: martijn.martijn, Assigned: alfredkayser)


(Keywords: crash, testcase)

Crash Data


(2 files, 3 obsolete files)

See upcoming testcase, which crashes Mozilla when clicking on the button.
Because of the use of enhanced privileges, you need to test it locally.
I'm obviously doing something wrong in the testcase, but Mozilla should give an error here, not crash.

Talkback ID: TB21864613G
MSVCR80.dll + 0x142d0 (0x602842d0)
XPTC_InvokeByIndex   XPCWrappedNative::CallMethod
Attached file testcase
What's wrong with crashing? Garbage in, crash out is not unreasonable in this case, since this is not exposed to web content. There are lots of cases where we crash if you pass null to functions that expect a value.
presumably this is strlen(0) fwiw.

what's wrong w/ crashing is that if chrome js screws up (which it will do often) and you have open unsaved browser state (comments in bugs or unfiled bugs, or chats or whatever), then you've lost it. if that's mail, then your mailbox might be corrupted and you'll certainly lose the essay you wrote.

if you want to stick up a dialog in debug mode to make people fix bad code, fine.

but i don't appreciate losing my work just because of someone's holy crusade for optimization. we have cycles to spare and our users care much more about not losing their data than they do about a few bytes here or there.
Despite my earlier statements to the contrary, I agree with timeless on this one.  Because of our extension model, we really should ensure that script cannot cause crashes.
Simple patch to ensure that parameters are valid to prevent crashes...

(But should not BaseHashtable also be protected by argument checking???)
Assignee: nobody → alfredkayser
Attachment #263512 - Flags: review?(darin.moz)
Previous patch is also still valid (public interface, so it should check arguments anway)
Attachment #263516 - Flags: review?(darin.moz)
Alfred, Darin isn't really available for review anymore, I suspect.
Attachment #263512 - Attachment is obsolete: true
Attachment #263516 - Attachment is obsolete: true
Attachment #263521 - Flags: review?(darin.moz)
Attachment #263512 - Flags: review?(darin.moz)
Attachment #263516 - Flags: review?(darin.moz)
Comment on attachment 263521 [details] [diff] [review]
Combined patch (and removed the LocalWin part)

Or Martijn, can you do the review?
Attachment #263521 - Flags: review?(darin.moz) → review?(benjamin)
if (!prop) return NS_ERROR_INVALID_ARG --> NS_ENSURE_ARG(prop)
Attachment #263521 - Attachment is obsolete: true
Attachment #263983 - Flags: review?
Attachment #263521 - Flags: review?(benjamin)
Attachment #263983 - Flags: review? → review?(darin.moz)
Attachment #263983 - Flags: review?(darin.moz) → review+
Attachment #263983 - Flags: superreview?(dougt)
Comment on attachment 263983 [details] [diff] [review]

did we fix BaseHashtable? 

This bandaide is fine, but the underlying crash should be addressed.

we should avoid crashing where possible as a rule.
Attachment #263983 - Flags: superreview?(dougt) → superreview+
Who can do the checkin?

As for BaseHashtable, I didn't dare to go yet in that direction because it is used to many places to face the risk of performance impact and such...
And then it seems to be actually in nsTHashtable.h, that GetEntry and PutEntry (and others) may need to be fixed:
Whiteboard: [checkin needed]
Note, there seems to be many crash bugs related to the hash code.
mozilla/xpcom/ds/nsProperties.cpp        1.56
mozilla/xpcom/io/nsDirectoryService.cpp  1.93
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha5
Is there a bug to address comments 12-13?
Flags: in-testsuite?
Bug 380607 created for comments 12-13
Crash Signature: [@ MSVCR80.dll]
You need to log in before you can comment on or make changes to this bug.