Closed Bug 1159409 Opened 5 years ago Closed 5 years ago

Don't register an Image with a ProgressTracker until the constructor finishes running

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(2 files)

From bug 1157343 comment 10:

We use ProgressTrackerInit to register the image with its ProgressTracker _while the image's constructor is running_. ProgressTracker can then both internally touch the refcount on the image and hand out a reference to the image via ProgressTracker::GetImage(). This is totally unsafe, because if we take such a reference and then drop it while the constructor is still running, the image's refcount can hit zero before it's been assigned to an nsRefPtr. We can end up running its destructor before the constructor even returns. This is very bad news.
In this part I remove Init() from the Image interface. This makes sense because:

1. We never call it via an Image* pointer, so it has no need to be virtual.

2. Only ImageFactory calls it, so it should probably be private to enforce that.
   (The classes that implement it are friends with ImageFactory.)

3. It's not great that we have to pointlessly implement Init() for ImageWrapper
   and DynamicImage, when nobody will ever it call in either case.

I'm doing this in the context of this bug because in part 2, MultipartImage is
going to need an Init() method, and it needs to have a different signature than
the existing implementations for RasterImage and VectorImage.
Attachment #8598978 - Flags: review?(tnikkel)
Assignee: nobody → seth
Status: NEW → ASSIGNED
Attachment #8598978 - Flags: review?(tnikkel) → review+
In this part we do the real work and remove ProgressTrackerInit.

ProgressTrackerInit had two responsibilities:

- On construction, it called ProgressTracker::SetImage() and
  Image::SetProgressTracker(). ImageFactory now handles both of those tasks.
  This also means that RasterImage and VectorImage no longer need to take a
  ProgressTracker parameter in their constructors.

- On destruction, it called ProgressTracker::ResetImage(). ImageResource now
  handles that for RasterImage and VectorImage. (It's the superclass of both.)
  MultipartImage is unfortunately not an ImageResource, so it needs to call
  ResetImage() in its destructor as well.

This was mostly a straightforward transformation, except for the MultipartImage
case, because MultipartImage did some things in its constructor that weren't
safe to do if it didn't already have a ProgressTracker. I've avoided that issue
by giving MultipartImage an Init() method. I've also added an ImageFactory
method to construct MultipartImage objects. That allows me to avoid making
MultipartImage a friend of ProgressTracker, and really it's how I should've done
it in the first place - we have ImageFactory for a reason.
Attachment #8598987 - Flags: review?(tnikkel)
Attachment #8598987 - Flags: review?(tnikkel) → review+
Thanks for the review!

Requesting checkin. The try job in comment 3 is green, and it includes both this patch and the patch in bug 1159502, which I'm also requesting checkin for.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/21735c9f4fae
https://hg.mozilla.org/mozilla-central/rev/dfa398350b4a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.