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

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Roland Mainz]

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() ... =:-)

Comment 1

[Roland Mainz]

Taking ...

Comment 2

[Roland Mainz]

Attachment: Patch for 2002-01-07-08-trunk

Comment 3

[Roland Mainz]

Who wants to r=/sr= this cleanup ?

Comment 4

[Simon Fraser [no longer active]]

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

sr=sfraser

Comment 5

[timeless]

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...

Comment 6

[Roland Mainz]

Attachment: 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) ...

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 9

[Roland Mainz]

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 ?

Comment 10

[Roland Mainz]

Marking FIXED for now...