Closed Bug 1232672 Opened 5 years ago Closed 5 years ago

Make hash table clients check for failure


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox46 --- fixed


(Reporter: jonco, Assigned: jonco)



(4 files)

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.
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.
Attachment #8699018 - Flags: review?(luke)
This found a couple of places in xpconnect that should check the return value.
Attachment #8699021 - Flags: review?(wmccloskey)
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.
Attachment #8699023 - Flags: review?(n.nethercote)
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.
Attachment #8699024 - Flags: review?(sphink)
Comment on attachment 8699018 [details] [diff] [review]

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

Attachment #8699018 - Flags: review?(luke) → review+
Comment on attachment 8699024 [details] [diff] [review]

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

Attachment #8699024 - Flags: review?(sphink) → review+
Comment on attachment 8699023 [details] [diff] [review]

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.
Attachment #8699023 - Flags: review?(n.nethercote) → review+
(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.
Attachment #8699021 - Flags: review?(wmccloskey) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.