Closed Bug 1361602 Opened 3 years ago Closed 2 months ago

Coverity report: nsMenuPopupFrame::​CreateWidgetForView(nsView *): Pointer is checked against null but then dereferenced anyway

Categories

(Core :: XUL, defect, P4, trivial)

defect

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: mats, Unassigned)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, good-first-bug)

Attachments

(1 file, 2 obsolete files)

Coverity CID 1122367 Dereference after null check

Either the check against null is unnecessary, or there may be a null pointer dereference.

In nsMenuPopupFrame::​CreateWidgetForView(nsView *): Pointer is checked against null but then dereferenced anyway



   6. var_compare_op: Comparing this->mContent to null implies that this->mContent might be null.
277  if (mContent && widgetData.mNoAutoHide) {
278    if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::titlebar,
279                              nsGkAtoms::normal, eCaseMatters)) {
280      widgetData.mBorderStyle = eBorderStyle_title;
281
282      mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::label, title);
283
284      if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::close,
285                                nsGkAtoms::_true, eCaseMatters)) {
286        widgetData.mBorderStyle =
287          static_cast<enum nsBorderStyle>(widgetData.mBorderStyle | eBorderStyle_close);
288      }
289    }
290  }
291
292  nsTransparencyMode mode = nsLayoutUtils::GetFrameTransparency(this, this);
   7. identity_transfer: Member function call this->GetContent() returns field mContent. [show details]
   CID 1122367 (#1 of 1): Dereference after null check (FORWARD_NULL)8. var_deref_model: Passing null pointer this->GetContent() to GetParent, which dereferences it. [show details]
I would like to work on this.
Please assign to me.


Thanks
Flags: needinfo?(mats)
Assignee: nobody → 1991manish.kumar
Flags: needinfo?(mats)
Only in nsMenuPopupFrame.cpp file?

(In reply to Mats Palmgren (:mats) from comment #1)
> We should remove the mContent null-check and replace any mContent with
> GetContent().
> http://searchfox.org/mozilla-central/rev/
> abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/layout/xul/nsMenuPopupFrame.cpp#252
Flags: needinfo?(mats)
Yeah, it's probably not worth doing en masse, but I figured
we might as well fix that here while touching this code.
Flags: needinfo?(mats)
So this kind of LOC:  

 'if (mContent && widgetData.mNoAutoHide) {
    if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::titlebar,
                              nsGkAtoms::normal, eCaseMatters)) '
will become 

 'if (GetContent() && widgetData.mNoAutoHide) {
    if (GetContent().AttrValueIs(kNameSpaceID_None, nsGkAtoms::titlebar,
                              nsGkAtoms::normal, eCaseMatters)) '

Please correct me If I am doing wrong?






(In reply to Mats Palmgren (:mats) from comment #1)
> We should remove the mContent null-check and replace any mContent with
> GetContent().
> http://searchfox.org/mozilla-central/rev/
> abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/layout/xul/nsMenuPopupFrame.cpp#252
Flags: needinfo?(mats)
The important bit is to remove the null-check of mContent in the first
if-condition, which is what Coverity complained about.
But yeah, all other instances of mContent should become GetContent()
+ fix the indentation.
Flags: needinfo?(mats)
Attached patch Patch_Bug1361602 (obsolete) — Splinter Review
Please review.

Thanks
Attachment #8964202 - Flags: review?(mats)
Comment on attachment 8964202 [details] [diff] [review]
Patch_Bug1361602

GetContent() return a pointer, so "GetContent()." won't compile.
Also, you forgot "()" in a few places, and you didn't fix 
the indentation as requested.

It's probably a good idea if you setup a local build and make
sure your changes compiles before requesting review:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Thanks!
Attachment #8964202 - Flags: review?(mats) → review-
Attached patch PatchV2_Bug1361602 (obsolete) — Splinter Review
Please review.

'./mach xpcshell-test services/sync/tests/unit'
--------------------------------------------------
xpcshell
~~~~~~~~
Ran 105 checks (105 tests)
Expected results: 103
Skipped: 2 tests
OK
---------------------------------------------------
Attachment #8964202 - Attachment is obsolete: true
Attachment #8964434 - Flags: review?(mats)
Attachment #8964434 - Flags: review?(enndeakin)
Comment on attachment 8964434 [details] [diff] [review]
PatchV2_Bug1361602

I don't think this needs two reviewers. Make sure to fix the indentation though

+          GetContent()->AsElement()->AttrValueIs(kNameSpaceID_None,
                                              nsGkAtoms::remote,
      
The second line should line up with the parenthesis.
Attachment #8964434 - Flags: review?(enndeakin)
Please review.
Attachment #8964434 - Attachment is obsolete: true
Attachment #8964434 - Flags: review?(mats)
Attachment #8964689 - Flags: review?(enndeakin)
Attachment #8964689 - Flags: review?(enndeakin) → review?(mats)
Comment on attachment 8964689 [details] [diff] [review]
PatchV3_Bug1361602

I don't see anything in this patch that corresponds to what the commit message says, so presumably somebody already fixed that part?
So you should change the commit message to describe what you're actually changing here.

AFAICT, this patch only replaces mContent with GetContent(), which is an idempotent change, so you might want to include "(idempotent patch)" at the end of the commit message to make that clear.

>-          mContent->AsElement()->AttrValueIs(kNameSpaceID_None,
>-                                             nsGkAtoms::remote,
>-                                             nsGkAtoms::_true, eIgnoreCase));
>+          GetContent()->AsElement()->AttrValueIs(kNameSpaceID_None, nsGkAtoms::remote, nsGkAtoms::_true, eIgnoreCase));

I don't think we should change the line-breaking like this.  In general, it's a good idea to follow the existing formatting, i.e. function arguments that are lined up on separate lines should continue to do so unless there's a reason not to.  Also, as a rule-of-thumb, we try to keep lines within 80 columns, unless it's motivated for readability reasons to make the lines longer (which it rarely is).

So please keep the general formatting as is, except for adding a few indentation
spaces so that arguments still align as noted in comment 10.
Attachment #8964689 - Flags: review?(mats) → review-
Assignee: 1991manish.kumar → nobody

Hi, I would like to take this bug. Is it still available?

Looks like bug 1423990 already fixed it, so, there isn't anything to do here.

Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.