1309082 - factor out nsDocument image tracking into a separate class

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

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.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: Bug 1309082 - Part 1: Extract image tracking from nsDocument into a separate, refcounted object.

This is refcounted as we'll need to hold strong references to the ImageTracker
from style structs that load images.

---
 dom/base/ImageTracker.cpp          | 161 +++++++++++++++++++++++++++++++++++++
 dom/base/ImageTracker.h            |  69 ++++++++++++++++
 dom/base/ScriptSettings.cpp        |   4 +-
 dom/base/moz.build                 |   2 +
 dom/base/nsDocument.cpp            | 142 +++-----------------------------
 dom/base/nsDocument.h              |  15 +---
 dom/base/nsIDocument.h             |  29 +------
 dom/base/nsImageLoadingContent.cpp |  24 +++---
 gfx/thebes/gfxSVGGlyphs.cpp        |   3 +-
 image/SVGDocumentWrapper.cpp       |   5 +-
 layout/base/nsPresShell.cpp        |   3 +-
 layout/style/nsStyleStruct.cpp     |   9 ++-
 12 files changed, 274 insertions(+), 192 deletions(-)
 create mode 100644 dom/base/ImageTracker.cpp
 create mode 100644 dom/base/ImageTracker.h

Review commit: https://reviewboard.mozilla.org/r/84822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84822/

Comment 2

[Cameron McCormack (:heycam)]

Attachment: Bug 1309082 - Part 2: Rename some ImageTracker members.

---
 dom/base/ImageTracker.cpp          | 48 +++++++++++++++++++-------------------
 dom/base/ImageTracker.h            | 16 ++++++-------
 dom/base/nsDocument.cpp            |  4 ++--
 dom/base/nsImageLoadingContent.cpp |  8 +++----
 gfx/thebes/gfxSVGGlyphs.cpp        |  2 +-
 image/SVGDocumentWrapper.cpp       |  4 ++--
 layout/base/nsPresShell.cpp        |  2 +-
 layout/style/nsStyleStruct.cpp     |  8 +++----
 8 files changed, 46 insertions(+), 46 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/84824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84824/

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Oh, it mainly involves nsDocument... I don't think I can review this actually. Redirecting to bholley.

Comment 4

[Bobby Holley (:bholley)]

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 5

[Bobby Holley (:bholley)]

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.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8799684 [details]
Bug 1309082 - Part 2: Rename some ImageTracker members.

https://reviewboard.mozilla.org/r/84824/#review83942

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8799683 [details]
Bug 1309082 - Part 1: Extract image tracking from nsDocument into a separate, refcounted object.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84822/diff/1-2/

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8799684 [details]
Bug 1309082 - Part 2: Rename some ImageTracker members.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84824/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73b3f8cc7d5f
https://hg.mozilla.org/mozilla-central/rev/3e153a5acd35