Open Bug 1268788 Opened 5 years ago Updated 17 days ago

Use [[nodiscard]] more in nsExpirationTracker

Categories

(Core :: XPCOM, task)

task
Not set
normal

Tracking

()

REOPENED
Tracking Status
firefox49 --- affected

People

(Reporter: n.nethercote, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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

As par bug 1268766 comment #13, we don't think anyone is going to fix this.
So we can close this bug.
If someone is interested in fixing this bug, please file a new bug.

Status: NEW → RESOLVED
Closed: 18 days ago
Resolution: --- → INVALID

That comment doesn't seem to apply here: this bug is about a specific class, not a directory....

Flags: needinfo?(t.matsuu)

(In reply to Boris Zbarsky [:bzbarsky] from comment #6)

That comment doesn't seem to apply here: this bug is about a specific class, not a directory....

Ah, sorry.
Reopened.

Status: RESOLVED → REOPENED
Flags: needinfo?(t.matsuu)
Resolution: INVALID → ---
You need to log in before you can comment on or make changes to this bug.