795703 - Do not assert when calling do_GetWeakReference() on a nsISupport not capable of that

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mounir Lamouri (:mounir)]

First of all, that means you have to check before calling this that you object actually supports weak reference but also means that in some situations, we will hit that assert unexpectedly like when removing an objet from an ObserverList which is no longer in the list. The nsISupport will be converted to a weak ref which is going to assert.

Ideally, we should let the caller to decide if a failure should assert or not.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 2

[Mounir Lamouri (:mounir)]

Sent to try with other patches:
https://tbpl.mozilla.org/?tree=Try&rev=6a6690d20239

Comment 3

[Doug Turner (:dougt)]

Some callers actually perform this assertion after the call:

http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsArray.cpp#112
http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsArray.cpp#158


Mounir, I am fine with this change.  Would you mind looking at the other callers.  Basically ensure that they will safely fail if the result of GetWeakReference is null.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 666334 [details] [diff] [review]
Patch

Useless assertion. If anything, it could be a warning, and I think even that
is wrong. So, r+

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Patch (r=smaug)

So I tried to check all callers (there are a tons of them) but I realized that we can't actually do the following pattern:
nsCOMPtr<nsIWeakReference> weakref = do_GetWeakReference(foo);
NS_ASSERTION(weakref, "blah!");

The reason being that would assert if |foo| was originally null. So, putting that assert would require us to read the method to understand if foo can actually be null. If foo can be null, there is no point in putting any assert because the code will more than likely handle a null weakref.

At that point I think we should land without checking all callers. It would take a crazy amount of time.

Comment 6

[Mounir Lamouri (:mounir)]

Patch is passing try:
https://tbpl.mozilla.org/?tree=Try&rev=b0a22f2fea28

Comment 7

[Doug Turner (:dougt)]

mounir, do you just want to land the r+ smaug patch (without any of the other call sites fixed?)

Comment 8

[Mounir Lamouri (:mounir)]

Landing the patch I attached would be fine (some more fixes/checks is better than none).
I asked you a review because smaug isn't an XPCOM peer. If you want to delegate your power to him, I'm fine with that :)

Comment 9

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5320ce41aa

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

bsmedberg has specifically told me not to do this patch in the past.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2b5320ce41aa