Fix the allocation handling of frame time recording

RESOLVED FIXED in mozilla30

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

unspecified
mozilla30
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
FramesTimingRecording::mIntervals should use fallible allocations, as
the code which is using it is prepared to handle OOM conditions.
However, this handling is not required for the array argument passed to
LayerManager::StopFrameTimeRecording, since that array is coming from
the IPDL protocol, and as of bug 967167 we handle the OOM conditions at
the IPDL protocol layer properly, so no extra OOM handling is necessary
here.
(Assignee)

Comment 1

4 years ago
Created attachment 8372836 [details] [diff] [review]
Fix the allocation handling of frame time recording

FramesTimingRecording::mIntervals should use fallible allocations, as
the code which is using it is prepared to handle OOM conditions.
However, this handling is not required for the array argument passed to
LayerManager::StopFrameTimeRecording, since that array is coming from
the IPDL protocol, and as of bug 967167 we handle the OOM conditions at
the IPDL protocol layer properly, so no extra OOM handling is necessary
here.
(Assignee)

Updated

4 years ago
Assignee: nobody → ehsan
Blocks: 969864
(Assignee)

Updated

4 years ago
Attachment #8372836 - Flags: review?(bgirard)
(Assignee)

Comment 2

4 years ago
Comment on attachment 8372836 [details] [diff] [review]
Fix the allocation handling of frame time recording

Avi: BenWa says you're a better reviewer here.

One thing to note in the patch is that the two hunks don't really have anything to do with each other.  For mIntervals, I was not sure what the expected size of the array is and whether is bounded, and chose the fallible array because there was code which tried to handle the OOM case.  If the size of the array is bounded, then we should make it infallible and remove that OOM handling code.
Attachment #8372836 - Flags: review?(bgirard) → review?(avihpit)
(Assignee)

Updated

4 years ago
Attachment #8372836 - Attachment is obsolete: true
Attachment #8372836 - Flags: review?(avihpit)
(Assignee)

Comment 3

4 years ago
Created attachment 8376336 [details] [diff] [review]
Fix the allocation handling of frame time recording; r=avi

The size of these arrays can be controlled by a pref, but the max size
is bound to 216k, therefore we do not need to do a fallible allocation
for them.
(Assignee)

Updated

4 years ago
Attachment #8376336 - Flags: review?(avihpit)

Comment 4

4 years ago
Comment on attachment 8376336 [details] [diff] [review]
Fix the allocation handling of frame time recording; r=avi

Review of attachment 8376336 [details] [diff] [review]:
-----------------------------------------------------------------

That's cleaner than before. Thanks!
Attachment #8376336 - Flags: review?(avihpit) → review+
(Assignee)

Comment 5

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/557578ef40b6
https://hg.mozilla.org/mozilla-central/rev/557578ef40b6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.