1290239 - HashNativeKey computes incorrect hash value when |mBaseSet| is null

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

The hashing function for XPCNativeSetKey can be called with a null mBaseSet. In this case, it is supposed to compute the hash value for a new XPCNativeSet with mAddition added. It does this by computing the hash for a native set containing _only_ mAddition, but all XPCNativeSets also contain nsISupports as the first value, so that must be added into the hash value, too. I think the worst that can happen is that sometimes we end up with two copies of a native set floating around.

Comment 1

[Andrew McCreight [:mccr8]]

In my patches for bug 1288909, I have to remove native sets from the native set map when they die, but this bug means that the map can contain a pointer to the set that is not removed when you call Remove() on that set.

Comment 2

[Andrew McCreight [:mccr8]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d84c376c45ad

Comment 3

[Andrew McCreight [:mccr8]]

You should review bug 1290614 first, if only to get a better sense of XPCNativeSetKey.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1290239, part 1 - Compute the correct hash value for an XPCNativeSetKey when mBaseSet is null.

The current hash function fails to take into account that all
XPCNativeSets have nsISupports as their first element.

Review commit: https://reviewboard.mozilla.org/r/70370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70370/

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1290239, part 2 - Add assertions about inserting into NativeSetMap.

If we are inserting an XPCNativeSet into a NativeSetMap, and we failed
to find it, then the insertion should not have found some other set
there already.

Additionally in this case, the hash value of a XPCNativeSetKey for the
set we are inserting should be the same as the key we used to insert
the set into the map. If this property does not hold, then we can't
use Remove() to remove the set from the map. This condition would have
caught the error that part 1 fixes.

Review commit: https://reviewboard.mozilla.org/r/70372/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70372/

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8779380 [details]
Bug 1290239, part 1 - Compute the correct hash value for an XPCNativeSetKey when mBaseSet is null.

https://reviewboard.mozilla.org/r/70370/#review67864

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8779381 [details]
Bug 1290239, part 2 - Add assertions about inserting into NativeSetMap.

https://reviewboard.mozilla.org/r/70372/#review67868

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e582ee72021c
part 1 - Compute the correct hash value for an XPCNativeSetKey when mBaseSet is null. r=mrbkap
https://hg.mozilla.org/integration/autoland/rev/4eda5a74d949
part 2 - Add assertions about inserting into NativeSetMap. r=mrbkap

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e582ee72021c
https://hg.mozilla.org/mozilla-central/rev/4eda5a74d949