Commit 4cbb8929 authored by Liquan(Max) Gu's avatar Liquan(Max) Gu Committed by Commit Bot

[FCP++] Prevent null from being trace time

We introduced a bug in https://chromium-review.googlesource.com/c/chromium/src/+/1380193.
When image paint detector finds the largest paint candidate with null time, it passes
in to the trace, which will cause a crash.

To fix it, we skip the candidate if the candidate's time is null.

Bug: 920627
Change-Id: Ie39ac9fbc1d09fc88e09851497a80610969bd527
Reviewed-on: https://chromium-review.googlesource.com/c/1407088Reviewed-by: default avatarSteve Kobes <skobes@chromium.org>
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622112}
parent f1674b1c
...@@ -137,6 +137,7 @@ void ImagePaintTimingDetector::PopulateTraceValue( ...@@ -137,6 +137,7 @@ void ImagePaintTimingDetector::PopulateTraceValue(
void ImagePaintTimingDetector::OnLargestImagePaintDetected( void ImagePaintTimingDetector::OnLargestImagePaintDetected(
ImageRecord* largest_image_record) { ImageRecord* largest_image_record) {
DCHECK(largest_image_record); DCHECK(largest_image_record);
DCHECK(!largest_image_record->first_paint_time_after_loaded.is_null());
largest_image_paint_ = largest_image_record; largest_image_paint_ = largest_image_record;
std::unique_ptr<TracedValue> value = TracedValue::Create(); std::unique_ptr<TracedValue> value = TracedValue::Create();
PopulateTraceValue(*value, *largest_image_record, PopulateTraceValue(*value, *largest_image_record,
...@@ -150,6 +151,7 @@ void ImagePaintTimingDetector::OnLargestImagePaintDetected( ...@@ -150,6 +151,7 @@ void ImagePaintTimingDetector::OnLargestImagePaintDetected(
void ImagePaintTimingDetector::OnLastImagePaintDetected( void ImagePaintTimingDetector::OnLastImagePaintDetected(
ImageRecord* last_image_record) { ImageRecord* last_image_record) {
DCHECK(last_image_record); DCHECK(last_image_record);
DCHECK(!last_image_record->first_paint_time_after_loaded.is_null());
last_image_paint_ = last_image_record; last_image_paint_ = last_image_record;
std::unique_ptr<TracedValue> value = TracedValue::Create(); std::unique_ptr<TracedValue> value = TracedValue::Create();
PopulateTraceValue(*value, *last_image_record, PopulateTraceValue(*value, *last_image_record,
...@@ -170,12 +172,16 @@ void ImagePaintTimingDetector::Analyze() { ...@@ -170,12 +172,16 @@ void ImagePaintTimingDetector::Analyze() {
// result unless it's a new candidate. // result unless it's a new candidate.
ImageRecord* largest_image_record = FindLargestPaintCandidate(); ImageRecord* largest_image_record = FindLargestPaintCandidate();
bool new_candidate_detected = false; bool new_candidate_detected = false;
if (largest_image_record && largest_image_record != largest_image_paint_) { if (largest_image_record &&
!largest_image_record->first_paint_time_after_loaded.is_null() &&
largest_image_record != largest_image_paint_) {
new_candidate_detected = true; new_candidate_detected = true;
OnLargestImagePaintDetected(largest_image_record); OnLargestImagePaintDetected(largest_image_record);
} }
ImageRecord* last_image_record = FindLastPaintCandidate(); ImageRecord* last_image_record = FindLastPaintCandidate();
if (last_image_record && last_image_record != last_image_paint_) { if (last_image_record &&
!last_image_record->first_paint_time_after_loaded.is_null() &&
last_image_record != last_image_paint_) {
new_candidate_detected = true; new_candidate_detected = true;
OnLastImagePaintDetected(last_image_record); OnLastImagePaintDetected(last_image_record);
} }
......
...@@ -70,6 +70,10 @@ class ImagePaintTimingDetectorTest ...@@ -70,6 +70,10 @@ class ImagePaintTimingDetectorTest
.id_record_map_.size(); .id_record_map_.size();
} }
void Analyze() {
return GetPaintTimingDetector().GetImagePaintTimingDetector().Analyze();
}
TimeTicks LargestPaintStoredResult() { TimeTicks LargestPaintStoredResult() {
ImageRecord* record = GetPaintTimingDetector() ImageRecord* record = GetPaintTimingDetector()
.GetImagePaintTimingDetector() .GetImagePaintTimingDetector()
...@@ -689,4 +693,13 @@ TEST_F(ImagePaintTimingDetectorTest, DeactivateAfterUserInput) { ...@@ -689,4 +693,13 @@ TEST_F(ImagePaintTimingDetectorTest, DeactivateAfterUserInput) {
EXPECT_EQ(CountRecords(), 0u); EXPECT_EQ(CountRecords(), 0u);
} }
TEST_F(ImagePaintTimingDetectorTest, NullTimeNoCrash) {
SetBodyInnerHTML(R"HTML(
<img id="target"></img>
)HTML");
SetImageAndPaint("target", 5, 5);
UpdateAllLifecyclePhasesForTest();
Analyze();
}
} // namespace blink } // namespace blink
...@@ -59,7 +59,6 @@ void TextPaintTimingDetector::OnLargestTextDetected( ...@@ -59,7 +59,6 @@ void TextPaintTimingDetector::OnLargestTextDetected(
const TextRecord& largest_text_record) { const TextRecord& largest_text_record) {
largest_text_paint_ = largest_text_record.first_paint_time; largest_text_paint_ = largest_text_record.first_paint_time;
largest_text_paint_size_ = largest_text_record.first_size; largest_text_paint_size_ = largest_text_record.first_size;
std::unique_ptr<TracedValue> value = TracedValue::Create(); std::unique_ptr<TracedValue> value = TracedValue::Create();
PopulateTraceValue(*value, largest_text_record, PopulateTraceValue(*value, largest_text_record,
largest_text_candidate_index_max_++); largest_text_candidate_index_max_++);
...@@ -90,12 +89,16 @@ void TextPaintTimingDetector::TimerFired(TimerBase* time) { ...@@ -90,12 +89,16 @@ void TextPaintTimingDetector::TimerFired(TimerBase* time) {
void TextPaintTimingDetector::Analyze() { void TextPaintTimingDetector::Analyze() {
TextRecord* largest_text_first_paint = FindLargestPaintCandidate(); TextRecord* largest_text_first_paint = FindLargestPaintCandidate();
bool new_candidate_detected = false; bool new_candidate_detected = false;
DCHECK(!largest_text_first_paint ||
!largest_text_first_paint->first_paint_time.is_null());
if (largest_text_first_paint && if (largest_text_first_paint &&
largest_text_first_paint->first_paint_time != largest_text_paint_) { largest_text_first_paint->first_paint_time != largest_text_paint_) {
OnLargestTextDetected(*largest_text_first_paint); OnLargestTextDetected(*largest_text_first_paint);
new_candidate_detected = true; new_candidate_detected = true;
} }
TextRecord* last_text_first_paint = FindLastPaintCandidate(); TextRecord* last_text_first_paint = FindLastPaintCandidate();
DCHECK(!last_text_first_paint ||
!last_text_first_paint->first_paint_time.is_null());
if (last_text_first_paint && if (last_text_first_paint &&
last_text_first_paint->first_paint_time != last_text_paint_) { last_text_first_paint->first_paint_time != last_text_paint_) {
OnLastTextDetected(*last_text_first_paint); OnLastTextDetected(*last_text_first_paint);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment