Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix fatal crash of ImageDecoderFixtureTest.ExifDataIsRespectedOnDecode #32356

Merged
zanderso merged 1 commit into from
Mar 31, 2022
Merged

Fix fatal crash of ImageDecoderFixtureTest.ExifDataIsRespectedOnDecode #32356

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/ui/painting/image_decoder_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ class TestIOManager final : public IOManager {
: nullptr),
unref_queue_(fml::MakeRefCounted<SkiaUnrefQueue>(
task_runner,
fml::TimeDelta::FromNanoseconds(0))),
fml::TimeDelta::FromNanoseconds(0),
gl_context_)),
Copy link
Member

@zanderso zanderso Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there are several places other than this in the code where a context is not passed to the SkiaUnrefQueue constructor. If failing to pass the context where needed can result in crashes, then that usage of optional arguments is probably not in agreement with the Google style guide. I'd like to hear thoughts from @jason-simmons on this.

Copy link
Contributor

@dnfield dnfield Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to making this a require parameter.

Copy link
Member

@jason-simmons jason-simmons Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it a required parameter would be ideal.

AFAICT making it optional is a workaround for scenarios like unit tests where it may be impractical to provide something resembling a GrContext. It would be preferable to avoid that if possible.

Copy link
Contributor

@dnfield dnfield Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class was written as a template class so that tests could explicitly create their own idea of what a context is.

We should make it non-optional and consider adding a DCHECK that it's not nullptr in the constructor.

runner_(task_runner),
is_gpu_disabled_sync_switch_(std::make_shared<fml::SyncSwitch>()),
weak_factory_(this) {
Expand Down