Closed Bug 1280676 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file)

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: 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).
I did indeed, and it does help. (no leaks during a Linux64 debug reftest run, with your patch combined with this one)
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)
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 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.
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
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)
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1280753 on adding a Close() call, too.
https://hg.mozilla.org/mozilla-central/rev/6e9946f0a1f6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: