215352 - NS_IMPL_QUERY_HEAD shouldn't null check out param

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

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

NS_IMPL_QUERY_HEAD shouldn't null check the out parameter, since there's no
need, and it's probably a significant waste of code size (considering the number
of QueryInterface implementations we have).

Comment 1

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

Attachment: patch

Comment 2

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

Comment on attachment 132000 [details] [diff] [review]
patch

I've been running with this patch for months.  Should be a noticeable codesize
savings since we use this macro so much.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 132000 [details] [diff] [review]
patch

how much to we save?

Comment 4

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

I don't know.  But why would we want this null check?

Comment 5

[Doug Turner (:dougt)]

I think we both know why -- calling QI with a null destination will crash.  I am
asking how much this check costs in footprint because if it only saved < 1k, i
wouldn't agree that we should removed.

Comment 6

[Brendan Eich [:brendan]]

dougt: only a C++ programmer could pass null for an out param pointer, and any
such C++ programmer deserves what he gets.  I think this holds for third-party
C++ code too.  If we have to defend against bogus nulls here, why not against
them in other cases?  Why not worry about bad (but non-null) pointers?  We can't
make the world safe for inept C++ coders, and we should not try.

/be

Comment 7

[Doug Turner (:dougt)]

why not remove all NS_ENSURE_ARG_POINTER's?  To heck with preconditions -- we
just need to remove the "inept" developers!

Quanitify the "significant waste of code size".   Maybe we can just keep this
check in DEBUG code. 

(also, the focus isn't on the "C++ programmer deserves what he gets", but rather
the user that gets Mozilla that may have components (hopefully just addons)
created by these "inept" coders.)

Comment 8

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

I think the general rule is that out params that are the equivalent of return
values in IDL (which this essentially is, despite the bizarre IDL signature of
QueryInterface) should not be null-checked.

Comment 9

[timeless]

brendan: really?
as a js programmer i tend to pass null, 0, and garbage to out parameters.

Comment 10

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

timeless: XPConnect doesn't allow you to pass null for out parameters that it
translates to return values.

Comment 11

[Brendan Eich [:brendan]]

NS_ENSURE_ARG_POINTER is a botch, IMHO.  Travis argued that no amount of code
bloat or cycle overhead was too much to defend against trivial null errors from
junior programmers hacking 2nd or 3rd party code that dynamically linked against
an XPCOM module.  I begged to differ, but that's all beside the point.  Out
params are not in params, and always require a valid address, which null is not.

timeless: you can't pass null as the out param *pointer* -- perhaps you're
thinking of the unused "in" value at the location addressed by the pointer?  For
inout, of course, that value must be set by the caller before it is used by the
callee.

/be

Comment 12

[Doug Turner (:dougt)]

I am in the same camp as travis on this one -- spend a few extra instructions to
protect the enduser from a crash is a good thing.  It is all to easy to get a
null address passed into QueryInterface.

Comment 13

[Brendan Eich [:brendan]]

> It is all to easy to get a null address passed into QueryInterface.

How?

Not if you use nsCOMPtrs. What do you mean, exactly?

I hope everyone is clear on out parameters, how they work with getter_AddRefs,
etc.  I hope I'm not missing something!

/be

Comment 14

[Doug Turner (:dougt)]

with nsCOMPtr's, you are safe.  

Comment 15

[David Bradley]

1. Does anyone know of a case where this actually caught a problem out in the wild?
2. I think probably the most common case of QI failure due to that pointer is
when the address space pointed to has been deleted, which this test wouldn't catch.
3. How often does someone pass in an address of a pointer where the pointer
wouldn't be either a local, nsCOMPtr, or data member in a class. Meaning passing
in a straight nsISupports ** pIFace where it could plausibly be null. If this
does happen it should assert, if people are ignoring those asserts, then I think
that's  a separate issue.
4. I wonder if there is any code that currently "relies" on this feature. I
don't think I've ever seen the assert fire other than possibly when I did
something stupid, locally, and quickly fix it.

Also such checks often hide the true source of the problem. Granted that's often
the fault of the error handling logic. But instead of the crash at source of the
problem, we get some crash at some other spot because something didn't handle
the QI, and then you have to figure that out.

Comment 16

[Brendan Eich [:brendan]]

> Also such checks often hide the true source of the problem. Granted that's
> often the fault of the error handling logic. But instead of the crash at
> source of the problem, we get some crash at some other spot because something
> didn't handle the QI, and then you have to figure that out.

Ding ding ding.

/me points at his nose, charades style

dougt, you say "uncle" yet?

/be

Comment 17

[Doug Turner (:dougt)]

no uncle yet.  why do we check for null after a malloc?  afterall, if malloc
fails, we should catch it at the first dereference, right?

I agreed in comment #7 that we could disable this check in an optimized build,
but we shouldn't stop checking for this logic error.  

Comment 18

[Mike Shaver (:shaver emeritus)]

null from malloc (where it can actually occur; it's possible that only Timeless
has ever seen it in the field) isn't something that can be prevented by care on
the part of the developer.  If it were possible for other system activity to
magically store zeros such that NULLs materialized unpredictably, I think that
would be a better analogy.

Comment 19

[Doug Turner (:dougt)]

what I am getting as is that we should test for known things that can fail. 
testing a malloc is a runtime precondition of dereference.  testing that the out
param can be written to is a logic precondition that also can be tested.

lets put this issue to bed -- leave it in for debug; out for opt.  first crash
in the wild gets me beers at the Tied House. 

Comment 20

[Alec Flett]

I'm going to have to agree with brendan on this one - I think that QI is a
special case in that it is a very very well understood API, and developers
should really know what they are doing when they call it.

In most cases, the caller to QI owns the pointer being passed in anyway, even if
they aren't using nsCOMPtr... i.e.

nsIFoo* foo;
bar->QI(..., (void**)&foo);

This is the only dangerous situation I see which might result in a null pointer
being passed into QI:

GetFoo(nsIFoo** aResult) {
  return mBar->QueryInterface(NS_GET_IID(nsIFoo), (void**)aResult);
}

I think that even in this case, it is the responsibility of GetFoo() to
null-check aResult if GetFoo() is part of some public API. Maybe we need to
educate people on this.

In any case, I think the well-understood nature of QI is what justifies us
dropping the null check. I haven't ever even seen one of these assertions.

Comment 21

[Brendan Eich [:brendan]]

Re: comment 17: malloc failure can happen, although not easily on modern
operating systems with VM (you swap to death first).  We want code that calls
malloc or a malloc wrapper to defend.  That's a completely different situation
from out param pointers, which cannot be null unless the caller passes null, or
is lucky in the garbage value passed via an uninitialized nsIFoo ** pointer --
but in either case, the caller is hopelessly confused.  An nsCOMPtr passed via
getter_AddRefs, or a raw ptr passed by its address (&rawptr), cannot result in
the out param pointer being null.

So the analogy is bogus.

> but we shouldn't stop checking for this logic error.  

*What* logic error?

/be

Comment 22

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

> I agreed in comment #7 that we could disable this check in an optimized build,
> but we shouldn't stop checking for this logic error.  

That's exactly what this patch does -- the assertion for DEBUG builds remains,
but the additional check for all builds is removed.  Unless you want the
behavior to actually differ between debug and non-debug builds, but I think
that's a bad idea.

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 132000 [details] [diff] [review]
patch

r=uncle.

Comment 24

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

Fix checked in to trunk, 2003-11-11 13:51 -0800.

Comment 25

[Bradley Baetz (:bbaetz)]

FWIW, it looks like this saved 10-20K according to tinderbox.

And we should in theory check for valid errors such as NULL returned from
malloc, just like we should catch exceptions thrown by c++ new. :)

Comment 26

[Doug Turner (:dougt)]

is there a bug to remove the preconditions per #11.  We may be able to save
another 20k.