Call OnPageHide() to pause SVG-in-opentype font documents

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
Right now, gfxSVGGlyphsDocument::~gfxSVGGlyphsDocument() stops the animations in the internal document by calling "Pause(nsSMILTimeContainer::PAUSE_PAGEHIDE)".

I believe it should instead explicitly call OnPageHide (as we do for SVG-as-an-image documents when they get cleaned up), which will implicitly invoke that same Pause() call (via a call to mAnimationController->OnPageHide()).

This will mean we can use "page hidden" state as a hint about whether the document is being used / kept-alive by a gfxSVGGlyphsDocument or not, in bug 741760.  And it will bring this code into closer alignment with SVGDocumentWrapper (for SVG-as-an-image).
Assignee

Updated

3 years ago
Assignee: nobody → dholbert
Did you perhaps push some patch to try already, and if so, does it help?

Since if not, I was thinking another option for bug 741760: figure out why something in the svg-as-image-document is addref/released and optimize that case out (if possible).
Assignee

Comment 2

3 years ago
I did indeed, and it does help. (no leaks during a Linux64 debug reftest run, with your patch combined with this one)
Assignee

Comment 4

3 years ago
Comment on attachment 8763319 [details]
Bug 1280676: When SVG-in-opentype font is being torn down, call OnPageHide() on its inner document instead of simply pausing animations.

Edwin, feel free to steal review if you're comfortable r+'ing this. You or birtles are probably the most appropriate reviewers, aside from roc.

This brings us into better alignment with the document teardown/cleanup code in our SVG-as-an-image code, which lives here:
http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/image/SVGDocumentWrapper.cpp#67

(This font code is missing a mViewer->Close() call that we make in the svg-as-an-image case; I'm not sure if we need that or not, but it's orthogonal to what I'm aiming to achieve in this bug.)

Note that the mDocument->OnPageHide() call that I'm adding will make the exact same controller->Pause() call that I'm removing, via a call to nsSMILAnimationController::OnPageHide():
http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/dom/base/nsDocument.cpp#9425
http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/dom/smil/nsSMILAnimationController.cpp#204
Attachment #8763319 - Flags: feedback?(edwin)
Assignee

Comment 5

3 years ago
Try runs:
 * Just smaug's patch from bug 741760 (leaks):
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba1d61f1b984

 * Smaug's patch from bug 741760, *plus* this bug's patch (no leaks):
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=acef246cd0c2

This patch lets smaug's patch not leak, because it lets him accurately tell the cycle collector to ignore these documents while they're in use by a font, and then start looking at them when the font goes away. (which his patch checks by testing whether the document is "visible" i.e. whether OnPageHide has been called)
Attachment #8763319 - Flags: review+
Comment on attachment 8763319 [details]
Bug 1280676: When SVG-in-opentype font is being torn down, call OnPageHide() on its inner document instead of simply pausing animations.

https://reviewboard.mozilla.org/r/59544/#review56654

Yeah, for consistency we should should do this. There will be more content-page-hidden notifications, but at least they are now consistently fired for all svg docs.

::: gfx/thebes/gfxSVGGlyphs.cpp:315
(Diff revision 1)
> -        }
>      }
>      if (mPresShell) {
>          mPresShell->RemovePostRefreshObserver(this);
>      }
>      if (mViewer) {

FWIW, as far as I see, we should call mViewer->Close() too to ensure we remove the listeners for "focus" and "blur". But not about this bug.

Comment 7

3 years ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e9946f0a1f6
When SVG-in-opentype font is being torn down, call OnPageHide() on its inner document instead of simply pausing animations. r=smaug
Assignee

Comment 8

3 years ago
Comment on attachment 8763319 [details]
Bug 1280676: When SVG-in-opentype font is being torn down, call OnPageHide() on its inner document instead of simply pausing animations.

Thanks! Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/6e9946f0a1f6
Attachment #8763319 - Flags: review?(bbirtles)
Attachment #8763319 - Flags: feedback?(edwin)
Assignee

Comment 9

3 years ago
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1280753 on adding a Close() call, too.

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e9946f0a1f6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.