Closed
Bug 664447
Opened 13 years ago
Closed 13 years ago
Make sure nsSMILAnimationController::OnPageHide is called
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
INVALID
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(1 file)
950 bytes,
patch
|
Details | Diff | Splinter Review |
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)
Comment 1•13 years ago
|
||
(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
Comment 2•13 years ago
|
||
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...)
Assignee | ||
Comment 3•13 years ago
|
||
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?
Assignee | ||
Comment 4•13 years ago
|
||
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.
Comment 5•13 years ago
|
||
(In reply to comment #3) > Destroy()ing the image's nsDocument > Do you mean Destroy() or delete? Sorry, you're right - I meant delete.
Assignee | ||
Comment 6•13 years ago
|
||
Note, the strong-parent-node disconnects/unlinks animationcontroller also in document's unlink.
Comment 7•13 years ago
|
||
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.
Assignee | ||
Comment 8•13 years ago
|
||
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)
Comment 9•13 years ago
|
||
Yup, agreed. (should this bug be closed, then?)
Assignee | ||
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•