Closed
Bug 1159409
Opened 9 years ago
Closed 9 years ago
Don't register an Image with a ProgressTracker until the constructor finishes running
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(2 files)
7.33 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
19.55 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → seth
Status: NEW → ASSIGNED
Updated•9 years ago
|
Attachment #8598978 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2e98a63812b
Updated•9 years ago
|
Attachment #8598987 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 4•9 years ago
|
||
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
Comment 5•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/21735c9f4fae https://hg.mozilla.org/integration/mozilla-inbound/rev/dfa398350b4a
Keywords: checkin-needed
Comment 6•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/21735c9f4fae https://hg.mozilla.org/mozilla-central/rev/dfa398350b4a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•