95902 - missing return NS_OK and memory leak if SetMediaText fails

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ulrich Drepper]

I just saw that the compiler is complaining about a missing return
incontent/html/style/src/nsCSSStyleSheet.cpp.  I'll attach a patch.

Comment 1

[Ulrich Drepper]

Attachment: Add missing return

Comment 2

[Ulrich Drepper]

*** Bug 95903 has been marked as a duplicate of this bug. ***

Comment 3

[Håkan Waara]

rubber-r=hwaara

Comment 4

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

r=dbaron, but while we're there, could we change the
  NS_ENSURE_ARG_POINTER(aInstancePtrResult);
to
  NS_ASSERTION(aInstancePtrResult, "null out param");
since we don't need to null-check out params in release builds.

We should probably also set *aInstancePtrResult to null for the error returns
(all 3, if the first one is removed).

Comment 5

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

Actually, we also leak on failure if SetMediaList fails.  Maybe the function
should read:

nsresult
NS_NewMediaList(nsIMediaList** aInstancePtrResult, const nsAReadableString&
aMediaText) {
  nsresult rv;
  NS_ASSERTION(aInstancePtrResult, "null out param");

  nsCOMPtr<nsISupportsArray> array;
  rv = NS_NewISupportsArray(getter_AddRefs(array));
  if (NS_FAILED(rv)) {
    *aInstancePtrResult = nsnull;
    return NS_ERROR_OUT_OF_MEMORY;
  }

  DOMMediaListImpl* medialist = new DOMMediaListImpl(array, nsnull);
  if (!mediaList) {
     *aInstancePtrResult = nsnull;
    return NS_ERROR_OUT_OF_MEMORY;
  }
  NS_ADDREF(*aInstancePtrResult = mediaList);

  rv = medialist->SetMediaText(aMediaText);
  if (NS_FAILED(rv))
    NS_RELEASE(*aInstancePtrResult);

  return rv;
}

although I hesitate to waste that much code on checking for out-of-memory when
we'd probably crash miserably elsewhere.

Comment 6

[Pierre Saslawsky]

Reassigned to Boris

Comment 7

[Boris Zbarsky [:bzbarsky]]

This will be fixed as part of the patch in bug 93977, since I'm touching a lot
of this code there anyway...

Comment 8

[Pierre Saslawsky]

It is in my patch for bug 93371 too.  One never knows... 

Comment 9

[Boris Zbarsky [:bzbarsky]]

True.  I should clarify that the fix for the leak when SetMediaText fails is in
my patch for bug 93977.  The fix for the NS_OK was already in there before this
got filed.  :)

So let's keep this open till one or the other gets checked in.

Comment 10

[Boris Zbarsky [:bzbarsky]]

fixed by checkin for bug 93977