Open Bug 1268788 Opened 5 years ago Updated 17 days ago
Use [[nodiscard]] more in ns
nsExpirationTracker::AddObject() should be marked with MOZ_MUST_USE. But currently there are a bunch of call sites that don't check the return value, in the following files: docshell/shistory/nsSHEntryShared.cpp dom/base/nsDocument.cpp dom/canvas/CanvasImageCache.cpp gfx/layers/client/TiledContentClient.cpp image/SurfaceCache.cpp layout/base/ActiveLayerTracker.cpp layout/generic/nsGfxScrollFrame.cpp layout/style/RuleProcessorCache.cpp We should fix them. Also, nsExpirationTracker::MarkUsed() is fallible because it calls AddObject(). But it only ever does that just after calling RemoveObject(), so perhaps it isn't fallible. None of its call sites check for failure.
bz, since we talked about this on IRC you might be interested in seeing this patch.
Attachment #8746957 - Flags: feedback?(bzbarsky)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
AddObject() only fails on OOM so I think we should consider making it infallible instead. Can we make it call AgeAllGenerations on OOM, then retry the operation and then intentionally crash if it still fails?
> AddObject() only fails on OOM More precisely, it fails on OOM or on reaching the max count for the tracker. And the latter may well be under direct content control (e.g. for the selector cache tracker). > Can we make it call AgeAllGenerations on OOM If we're willing to do _that_, we can also just have it expire the thing being added on oom, right? We'd have to make sure all the callers can handle that.
Attachment #8746957 - Flags: feedback?(bzbarsky) → feedback+
> we can also just have it expire the thing being added on oom, right? Sure, if there is no advantage in compacting and expiring old objects first, we might as well expire the AddObject argument directly, but... > We'd have to make sure all the callers can handle that. Looking at the patch it seems like that actually is a problem. I think what I suggested avoids this problem, unless we AddObject two or more objects at some point (seems unlikely though).
Type: defect → task
Summary: Use MOZ_MUST_USE more in nsExpirationTracker → Use [[nodiscard]] more in nsExpirationTracker
Assignee: n.nethercote → nobody
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 18 days ago
Resolution: --- → INVALID
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
You need to log in before you can comment on or make changes to this bug.