Closed Bug 1309082 Opened 3 years ago Closed 3 years ago

factor out nsDocument image tracking into a separate class

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(2 files)

In stylo, we need to be able to untrack images on the main thread when Gecko style structs are destroyed.  We call Gecko style struct destructors from the Drop impl on the object that wraps the Gecko struct, and at this point (a) we might not be on the main thread, and (b) we don't have an nsPresContext around to pass in to the UntrackImages() functions.  But if we just store raw nsPresContext pointers on the style structs that store images and just dispatch a main thread runnable to call UntrackImages(), our nsPresContext might have already been destroyed.  Keeping a strong reference do the nsPresContext seems to hold things alive longer than they should be.

An alternative approach is to store a strong reference to the image tracking table from the relevant Gecko style structs.  That table doesn't refer back to the pres context or document, so we don't keep things alive too long, but we are able to keep the image tracking table alive long enough for us to be able to call UntrackImgaes() in the case where the structs are being destroyed due to the document being torn down.  So in this bug we're factoring out the image tracking table into a thread-safely refcounted object that nsStyleBackground etc. can hold on to.
Oh, it mainly involves nsDocument... I don't think I can review this actually. Redirecting to bholley.
Attachment #8799683 - Flags: review?(xidorn+moz) → review?(bobbyholley)
Attachment #8799684 - Flags: review?(xidorn+moz) → review?(bobbyholley)
Comment on attachment 8799683 [details]
Bug 1309082 - Part 1: Extract image tracking from nsDocument into a separate, refcounted object.

https://reviewboard.mozilla.org/r/84822/#review83936

::: dom/base/ImageTracker.h:40
(Diff revision 1)
> +public:
> +  ImageTracker();
> +  ImageTracker(const ImageTracker&) = delete;
> +  ImageTracker& operator=(const ImageTracker&) = delete;
> +
> +  NS_INLINE_DECL_REFCOUNTING(ImageTracker)

This needs to be THREADSAFE_REFCOUNTING right?

::: dom/base/nsIDocument.h:2354
(Diff revision 1)
> -
> -  // Makes the images on this document locked/unlocked. By default, the locking
> -  // state is unlocked/false.
> -  virtual nsresult SetImageLockingState(bool aLocked) = 0;
>  
> +  virtual mozilla::dom::ImageTracker* ImageTracker() = 0;

Is there a good reason for this to be virtual? Why can't we just put the ImageTracker member on nsIDocument and save all the virtual calls?
Comment on attachment 8799683 [details]
Bug 1309082 - Part 1: Extract image tracking from nsDocument into a separate, refcounted object.

r=me with those two fixes.
Attachment #8799683 - Flags: review?(bobbyholley) → review+
Comment on attachment 8799684 [details]
Bug 1309082 - Part 2: Rename some ImageTracker members.

https://reviewboard.mozilla.org/r/84824/#review83942
Attachment #8799684 - Flags: review?(bobbyholley) → review+
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73b3f8cc7d5f
Part 1: Extract image tracking from nsDocument into a separate, refcounted object. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e153a5acd35
Part 2: Rename some ImageTracker members. r=bholley
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8799683 - Flags: review?(bobbyholley)
https://hg.mozilla.org/mozilla-central/rev/73b3f8cc7d5f
https://hg.mozilla.org/mozilla-central/rev/3e153a5acd35
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.