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

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Daniel Holbert [:dholbert]]

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).

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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).

Comment 2

[Daniel Holbert [:dholbert]]

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

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Bug 1280676: When SVG-in-opentype font is being torn down, call OnPageHide() on its inner document instead of simply pausing animations.

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

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Daniel Holbert [:dholbert]]

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)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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

[Pulsebot]

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

Comment 8

[Daniel Holbert [:dholbert]]

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

Comment 9

[Daniel Holbert [:dholbert]]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e9946f0a1f6