useless null check in nsDestroyJSPrincipals

RESOLVED FIXED

Status

()

RESOLVED FIXED
13 years ago
2 months ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
coverity
Points:
---
Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

13 years ago
 
(Assignee)

Comment 1

13 years ago
Created attachment 218633 [details] [diff] [review]
drop null check
Attachment #218633 - Flags: superreview?(dveditz)
Attachment #218633 - Flags: review?(dbaron)
The null check was added to stop a free memory write (bug 52382) -- are we sure it's no longer needed?

Coverity is simply complaining about the inconsistency, it's possible it's the unchecked AddRef that's wrong. Then again if that's true then ::Subsumes and probably other places are equally dangerous.
(Assignee)

Comment 3

13 years ago
i'm definitely not sure. hence the review requests to the relevant parties.
(In reply to comment #2)
> The null check was added to stop a free memory write (bug 52382) -- are we sure
> it's no longer needed?

That's not where the null-check was added -- that's just where we changed from hiding the null-check within NS_IF_RELEASE since the null-assignment (after the release) within NS_IF_RELEASE was causing a free memory write.
It's been null-checked, perhaps reflexively, since the beginning (revision 1.1.2.1).
Comment on attachment 218633 [details] [diff] [review]
drop null check

I'd be slightly happier if nsJSPrincipals::Init() returned an error (or even asserted) when passed a null aPrincipal, but we must not ever do so or we'd be crashing.

sr=dveditz
Attachment #218633 - Flags: superreview?(dveditz) → superreview+
Comment on attachment 218633 [details] [diff] [review]
drop null check

r=dbaron
Attachment #218633 - Flags: review?(dbaron) → review+
a1.9?
Flags: blocking1.9?

Updated

11 years ago
Attachment #218633 - Flags: approval1.9+
Keywords: checkin-needed
Flags: blocking1.9? → blocking1.9-
Keywords: checkin-needed
OS: Mac OS X → All
QA Contact: caps
Hardware: Macintosh → All
Whiteboard: [needs landing]
(Assignee)

Comment 9

11 years ago
mozilla/caps/src/nsJSPrincipals.cpp 	1.27
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Summary: hopefully useless null check in nsDestroyJSPrincipals → useless null check in nsDestroyJSPrincipals
Whiteboard: [needs landing]
Blocks: 1230156
You need to log in before you can comment on or make changes to this bug.