1232672 - Make hash table clients check for failure

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Following on from bug 1231224 which related to Vectors, we should make clients of our hash tables check for possible OOM by adding MOZ_WARN_UNUSED_RESULT in appropriate places.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: warn-hashtable-return-values

Add MOZ_WARN_UNUSED_RESULT to HashMap and HasSet methods that need to be checked for failure and fix up the rest of the codebase.

This didn't catch anything too major under js/src: a couple of issues in SPSProfiler, one in the GC pre-barrier verifier and a bunch of unchecked uses in the jsapi tests.  I did have to change places where we deliberately ignore the return code to assign the result to a variable and then cast that to void.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: warn-hashtable-return-values-xpconnect

This found a couple of places in xpconnect that should check the return value.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: warn-hashtable-return-values-dmd

Unfortunately I had to add MOZ_ALWAYS_TRUE() around all uses of these hash table methods here.  The allocator itself is infallible so they can never return false, but we need to check the result otherwise we get an error.

I still think it's worth doing this so we can force the check in the more usual situation where allocation is fallible.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: warn-ordered-hashtable-return-values

Same thing for ordered hash tables.  This catches a couple of places in GC where we don't check the return code, in particular when clearing the weak keys table which can fail and leave the table with out-of-date entries.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8699018 [details] [diff] [review]
warn-hashtable-return-values

Review of attachment 8699018 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

Comment 6

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8699024 [details] [diff] [review]
warn-ordered-hashtable-return-values

Review of attachment 8699024 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8699023 [details] [diff] [review]
warn-hashtable-return-values-dmd

Review of attachment 8699023 [details] [diff] [review]:
-----------------------------------------------------------------

It's a shame these changes are necessary given that DMD uses an infallible AllocationPolicy, but I guess it's unavoidable.

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> It's a shame these changes are necessary given that DMD uses an infallible
> AllocationPolicy, but I guess it's unavoidable.

IIRC the infallible versions of nsTArray methods do something clever with templates to avoid this issue but I don't remember the specifics.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c46eebf3397e
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd740170e039

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c46eebf3397e
https://hg.mozilla.org/mozilla-central/rev/dd740170e039