Open
Bug 1268788
Opened 8 years ago
Updated 1 year ago
Use [[nodiscard]] more in nsExpirationTracker
Categories
(Core :: XPCOM, task)
Core
XPCOM
Tracking
()
REOPENED
Tracking | Status | |
---|---|---|
firefox49 | --- | affected |
People
(Reporter: n.nethercote, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
9.59 KB,
patch
|
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
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.
![]() |
Reporter | |
Comment 1•8 years ago
|
||
bz, since we talked about this on IRC you might be interested in seeing this patch.
Attachment #8746957 -
Flags: feedback?(bzbarsky)
![]() |
Reporter | |
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
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?
![]() |
||
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.
![]() |
||
Updated•8 years ago
|
Attachment #8746957 -
Flags: feedback?(bzbarsky) → feedback+
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).
Updated•3 years ago
|
Type: defect → task
Summary: Use MOZ_MUST_USE more in nsExpirationTracker → Use [[nodiscard]] more in nsExpirationTracker
![]() |
Reporter | |
Updated•3 years ago
|
Assignee: n.nethercote → nobody
Status: ASSIGNED → NEW
Comment 5•3 years ago
|
||
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: 3 years ago
Resolution: --- → INVALID
![]() |
||
Comment 6•3 years ago
|
||
That comment doesn't seem to apply here: this bug is about a specific class, not a directory....
Flags: needinfo?(t.matsuu)
Comment 7•3 years ago
|
||
(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 → ---
Updated•1 year ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•