Closed Bug 795703 Opened 8 years ago Closed 8 years ago
Do not assert when calling do
_Get Weak Reference() on a ns ISupport not capable of that
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.
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #666334 - Flags: review?(bugs)
Sent to try with other patches: https://tbpl.mozilla.org/?tree=Try&rev=6a6690d20239
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 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+
Attachment #666334 - Flags: review?(bugs) → review+
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.
Patch is passing try: https://tbpl.mozilla.org/?tree=Try&rev=b0a22f2fea28
mounir, do you just want to land the r+ smaug patch (without any of the other call sites fixed?)
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 :)
Attachment #666501 - Flags: review?(doug.turner) → review+
Target Milestone: --- → mozilla18
bsmedberg has specifically told me not to do this patch in the past.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.