Closed Bug 795703 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 622218, 774259, 622764
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #666334 - Flags: review?(bugs)
Attachment #666334 - Flags: review?(doug.turner)
Blocks: 795725
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+
Attached patch Patch (r=smaug)Splinter 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.
Attachment #666334 - Attachment is obsolete: true
Attachment #666334 - Flags: review?(doug.turner)
Attachment #666501 - Flags: review?(doug.turner)
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5320ce41aa
Flags: in-testsuite-
Target Milestone: --- → mozilla18
bsmedberg has specifically told me not to do this patch in the past.
https://hg.mozilla.org/mozilla-central/rev/2b5320ce41aa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.