If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make sure nsSMILAnimationController::OnPageHide is called

RESOLVED INVALID

Status

()

Core
SVG
RESOLVED INVALID
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 539546 [details] [diff] [review]
patch

This is a bit hackish, but at least makes sure OnPageHide is called even
if animation controller is create after the pagehide for the document, or
if there are other weird cases...
Attachment #539546 - Flags: review?(dholbert)
(In reply to comment #0)
> if animation controller is create after the pagehide for the document

I don't know how that would happen, but even if it can somehow happen, I don't think it'd be a problem (though we might need an assertion-tweak; see below).

All we really care about here is that the animation controller is *paused* when we hit Disconnect() (to prevent issues like bug 608295, where we get refresh-driver callbacks after the Disconnect).

nsSMILAnimationController *starts out* paused when created (with flag PAUSE_BEGIN), until we fire SVGLoad.  So I would expect that if we were to somehow create a controller between PageHide and document destruction, it'd *still* be in that paused state when we hit the destruction code.

If it turns out that this can happen, then we should just expand the scope of the abort to also accept PAUSE_BEGIN.
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Sorry, Comment 1 was a bit wrong -- I was mixing up the document's animation controller (which Begin()s automatically, clearing its PAUSE_BEGIN state, at construction-time) with the <svg> fragment's timed document root (which waits for SVGLoad before it calls Begin()).


So -- assuming "mIsShowing" correctly reflects our PageHide state, this still should never be a problem in the general (non-svg-as-an-image) case, because GetAnimationController() force-OnPageHide-pauses its newly-created animation controller if we're hidden.

It doesn't do that for images, though, because our image's nsDocument doesn't directly know whether it's visible (as an image) or not.

SO: we should only need to  handle this in the SVG-as-an-image case.  There, the question is, why would we be Destroy()ing the image's nsDocument before SVGDocumentWrapper::DestroyViewer()'s OnPageHide call?   (Presumably that's got to be what's happening somehow...)
I pushed a bit different strong-parent-node to tryserver.
That is without unlink animation controller from document.
That might be enough to fix this.

(In reply to comment #2)
> SO: we should only need to  handle this in the SVG-as-an-image case.  There,
> the question is, why would we be Destroy()ing the image's nsDocument before
> SVGDocumentWrapper::DestroyViewer()'s OnPageHide call?   (Presumably that's
> got to be what's happening somehow...)
Do you mean Destroy() or delete?
But the tryserver build may just leak the world, since there is a
different leak regression related to cycle collections during shutdown :(

I'll try to repush once that regression is fixed.
(In reply to comment #3)
> Destroy()ing the image's nsDocument
> Do you mean Destroy() or delete?

Sorry, you're right - I meant delete.
Note, the strong-parent-node disconnects/unlinks animationcontroller also in
document's unlink.
Ah, that clarifies things a bit.  So that may be the source of this stray Disconnect(), rather than the nsDocument destructor being the source.

Still, I don't understand why we'd be cycle collecting our image's nsDocument before SVGDocumentWrapper::DestroyViewer gets called...

Regardless - per irc discussion, I think I agree with the beginning of Comment 3 that the nsDocument probably doesn't need to unlink its animation controller.
Comment on attachment 539546 [details] [diff] [review]
patch

So, I assume this will not  be needed after all if Disconnect is called
only in the Document dtor.
Attachment #539546 - Flags: review?(dholbert)
Yup, agreed.  (should this bug be closed, then?)
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.