81289 - nsIDocument::GetDocumentURL() encourages leaky behaviour

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Simon Fraser [no longer active]]

nsIDocument has a GetDocumentURL method that returns in its return value an 
addreffed nsIURI. LXRing for use finds several leaks, because of the common 
pattern:

nsCOMPtr<nsIURI> uri = doc->GetDocumentURL()

Comment 1

[Johnny Stenback (:jst)]

There's a bunch of methods in nsIDocument that returns addref'ed pointers, if we
change one, we should change them all, IMO. I'm all for doing this.

Comment 2

[David Hyatt]

I humbly beg you to save this for 0.9.2 (after I land my branch).  :)

Comment 3

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

I'm willing to do this if jst isn't, although I'm not completely sure why it was
assigned to me.  I'm *un*realistically marking it 0.9.2.

Comment 4

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

Would anybody object if I fixed nsIScriptContext::GetGlobalObject at the same time?

Comment 5

[Johnny Stenback (:jst)]

nsIScriptContext::GetGlobalObject() too is fine with me.

Comment 6

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

Attachment: patch

Comment 7

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

Attachment: updated patch

Comment 8

[jag (Peter Annema)]

Index: mozilla/content/base/src/nsContentUtils.cpp

 nsContentUtils::GetDynamicScriptGlobal(JSContext* aContext,
                                       nsIScriptGlobalObject** aNativeGlobal)
 {
-  nsIScriptGlobalObject* nativeGlobal = nsnull;
   nsCOMPtr<nsIScriptContext> scriptCX;
   GetDynamicScriptContext(aContext, getter_AddRefs(scriptCX));
-  if (scriptCX) {
-    *aNativeGlobal = nativeGlobal = scriptCX->GetGlobalObject();
-  }
-  return nativeGlobal ? NS_OK : NS_ERROR_FAILURE;
+  if (!scriptCX)
+    return NS_ERROR_NULL_POINTER;
+  return scriptCX->GetGlobalObject(aNativeGlobal);
 }  

I think you should return NS_ERROR_FAILURE there.
 

Index: mozilla/content/xul/content/src/nsXULTreeElement.cpp

Watch your whitespace.


Index: mozilla/dom/src/base/nsJSEnvironment.cpp

+NS_IMETHODIMP
+nsJSContext::GetGlobalObject(nsIScriptGlobalObject** aGlobalObject)
 {
   JSObject *global = ::JS_GetGlobalObject(mContext);
+  *aGlobalObject = nsnull;
 
   if (!global) {
-    return nsnull;
+    return NS_ERROR_NULL_POINTER;

Here too just return NS_ERROR_FAILURE.

 if (!c || ((~c->flags) & (JSCLASS_HAS_PRIVATE |
                             JSCLASS_PRIVATE_IS_NSISUPPORTS))) {
-    return nsnull;
+    return NS_ERROR_NULL_POINTER;

And here too.

+  if (!native) {
+    return NS_ERROR_NULL_POINTER;
   }

...



Index: mozilla/dom/src/base/nsJSUtils.cpp

see nsContentUtils.



Index: mozilla/layout/xul/base/src/outliner/src/nsOutlinerSelection.cpp

whitespace...

Comment 9

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

Attachment: patch addressing jag's comments

Comment 10

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

I'm still not completely sure whether I should be returning NS_OK with a null
out param or whether I should be returning a failure status.  Perhaps it depends
on whether null is normal or an error, but I'm not sure what it is in these
cases.  As jag pointed out, there might also be some places where I could
propagate return values better.

Comment 11

[Johnny Stenback (:jst)]

IMO returning NS_OK in all (there might be some exceptions, of course) failure
cases in the implementations that are changed here is the right thing to do,
that's exactly what the old code did (effectively). For instance, I don't think
nsJSContext::GetGlobalObject() should ever return anything but NS_OK, the
question we're asking is, give me the nsIScriptGlobalObject that's the private
data in the global object in the context, if there is no global object, or if
the private data in the global is not of the currect type we simply give back a
null nsIScriptGlobalObject and return NS_OK. That will answer the same question
the old code answered, without intorducing random "exceptions". Only return
error codes in truly exceptional circumstances, such as failure to allocate
memory n' so on...

You might choose to warn (i.e. NS_WARNING) if there's no private data in the
global and in all the other cases where you currently return error codes. This
would help developers debug potential problems. Think of returning an error code
as if you'd be throwing an exception if we'd support exceptions, would it make
sense to throw an exception in all the cases where where we currently return
error codes (not only in this patch), I don't think so.

More comments coming up...

Comment 12

[Johnny Stenback (:jst)]

nsContentUtils::GetDynamicScriptGlobal() needs to initialize *aNativeGlobal to
nsnull (it didn't before, but it should), and IMO that method should never
return a failure code either, but let's not change that now...

nsJSContext::GetGlobalObject() should always return NS_OK.

Other than that, sr=jst

Comment 13

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

Attachment: patch addressing jst's comments

Comment 14

[jag (Peter Annema)]

r=jag

Comment 15

[Johnny Stenback (:jst)]

sr=jst, check this in!

Comment 16

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 17

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

Fix checked in 2001-06-19 20:26/20:27 PDT.

Comment 18

[Chris Petersen]

Marking verified per last comments.