nsBaseWidget::GetRenderingContext() does not correctly check for errors

RESOLVED FIXED in mozilla0.9.8

Status

()

--
critical
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: roland.mainz, Assigned: roland.mainz)

Tracking

({crash})

Trunk
mozilla0.9.8
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

17 years ago
nsBaseWidget::GetRenderingContext() is a pain, it does not correctly check for
errors, causing crashes in some places (no details, just look at the source of
nsBaseWidget::GetRenderingContext() ... =:-)
(Assignee)

Comment 1

17 years ago
Taking ...
Assignee: jaggernaut → Roland.Mainz
Severity: normal → critical
Keywords: crash
Target Milestone: --- → mozilla0.9.8
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

17 years ago
Created attachment 64444 [details] [diff] [review]
Patch for 2002-01-07-08-trunk
(Assignee)

Comment 3

17 years ago
Who wants to r=/sr= this cleanup ?
Keywords: patch, review

Comment 4

17 years ago
Comment on attachment 64444 [details] [diff] [review]
Patch for 2002-01-07-08-trunk

sr=sfraser
Attachment #64444 - Flags: superreview+

Comment 5

17 years ago
Comment on attachment 64444 [details] [diff] [review]
Patch for 2002-01-07-08-trunk

fix the else after return

and i'm not certain about addrefing the context...
(Assignee)

Comment 6

17 years ago
Created attachment 64587 [details] [diff] [review]
Better patch per timeless comments

I fixed the missing brackes around the |else| and added a comment why I am
addref'ing the object before we returning it (otherwise the nsCOMPtr would
destroy the nsIRenderingContext object and the function would return a ref to a
dead object) ...
Attachment #64444 - Attachment is obsolete: true
Is there any reason you have to keep indenting?  I'd much rather see this
written as:

nsIRenderingContext *rc;
nsresult rv = CallCreateInstance(kRenderingContextCID, &rc);
if (NS_FAILED(rv))
  return nsnull;

rv = rc->Init(mContext, this);
if (NS_FAILED(rv)) {
  NS_RELEASE(rc);
  return nsnull;
}

return rc;

which is shorter, less indented (and the *normal* code path is never indented),
and doesn't clutter things up with NS_WARNINGs for things that are never going
to happen.
Of course, if you wanted to make some more useful changes, you could change it
(and the callers) so it doesn't return an AddRef'd pointer.
(Assignee)

Comment 9

17 years ago
dbaron:
Valid arguments... problem: The patch was checked in 30mins ago... ;-(

Dou you want this issue fixed (well, the original issue of this bug was to get
rid of the silly crash) or can I put this to my
check-for-such-coding-stlyes-in-the-next-patch list ?
(Assignee)

Comment 10

17 years ago
Marking FIXED for now...
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.