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

RESOLVED FIXED in mozilla18

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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)

Updated

6 years ago
Blocks: 622218, 774259, 622764
(Assignee)

Comment 1

6 years ago
Created attachment 666334 [details] [diff] [review]
Patch
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #666334 - Flags: review?(bugs)
(Assignee)

Updated

6 years ago
Attachment #666334 - Flags: review?(doug.turner)
(Assignee)

Updated

6 years ago
Blocks: 795725
(Assignee)

Comment 2

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

Comment 3

6 years ago
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

6 years ago
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+
(Assignee)

Comment 5

6 years ago
Created attachment 666501 [details] [diff] [review]
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.
Attachment #666334 - Attachment is obsolete: true
Attachment #666334 - Flags: review?(doug.turner)
Attachment #666501 - Flags: review?(doug.turner)

Comment 7

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

Comment 8

6 years ago
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 :)

Updated

6 years ago
Attachment #666501 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 9

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.