Nicholas Nethercote [inactive] Reporter
	Description • 8 years ago

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.

	Nicholas Nethercote [inactive] Reporter
	Comment 1 • 8 years ago

bz, since we talked about this on IRC you might be interested in seeing this
patch.

	Mats Palmgren (inactive)
	Comment 2 • 8 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 8 years ago

> 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.

	Mats Palmgren (inactive)
	Comment 4 • 8 years ago

> 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).