Closed Bug 1417354 Opened 2 years ago Closed 2 years ago

Remove element in mElementsToRestyle that the element has been detached from document between Animation tick and styling

Categories

(Core :: DOM: Animation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(6 files)

Attached file An example
Attaching file is an example. What's going on there is;

1) Create an element and create a script animation on the element
2) Animation::Tick happens, then RequestRestyle(Standard) is called
3) The element is stored in mElementsToRestyle
4) In a requestAnimationFrame callback, detach the element from document
5) The element is not traversed since the element in not in the document tree
6) In a later requestAnimationFrame callback, attach the element to the document
7) In the first PreTraverse() we try to post animation restyle hint
   (It results an animation restyle marker)
8) The animation restyle hint does affect nothing since the element has no servo style data that has been already dropped when the element is detached

So the remaining element in mElementsToRestyle will not be a problem, but if we don't know this fact, there are cases that we can't understand what happens.  Actually I noticed this fact when I am trying patches in bug 1416966 (adding micro task check point on Animation.ready).
Priority: -- → P3
Blocks: 1425009
Blocks: 1388557
Comment on attachment 8952027 [details]
Bug 1417354 - Introduce nsIDocument::GetPresContext().

https://reviewboard.mozilla.org/r/221242/#review227068


Code analysis found 2 defects in this patch:
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/base/nsGlobalWindowInner.cpp:4791
(Diff revision 1)
>    }
>  
>    // Get a presentation shell for use in creating the hashchange event.
>    NS_ENSURE_STATE(IsCurrentInnerWindow());
>  
> -  nsIPresShell *shell = mDoc->GetShell();
> +  RefPtr<nsPresContext> presContext = mDoc->GetPresContext();

Error: Unused "kungfudeathgrip" 'refptr<nsprescontext>' objects constructed from temporary values are prohibited [clang-tidy: mozilla-kungfu-death-grip]

::: dom/base/nsGlobalWindowInner.cpp:4830
(Diff revision 1)
>    nsCOMPtr<nsIVariant> stateObj;
>    rv = mDoc->GetStateObject(getter_AddRefs(stateObj));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Obtain a presentation shell for use in creating a popstate event.
> -  nsIPresShell *shell = mDoc->GetShell();
> +  RefPtr<nsPresContext> presContext = mDoc->GetPresContext();

Error: Unused "kungfudeathgrip" 'refptr<nsprescontext>' objects constructed from temporary values are prohibited [clang-tidy: mozilla-kungfu-death-grip]
(In reply to Code Review Bot [:reviewbot] from comment #5)
> Comment on attachment 8952027 [details]
> Bug 1417354 - Introduce nsIDocument::GetPresContext().
> 
> https://reviewboard.mozilla.org/r/221242/#review227068
> 
> 
> Code analysis found 2 defects in this patch:
>  - 2 defects found by clang-tidy
> 
> You can run this analysis locally with:
>  - `./mach static-analysis check path/to/file.cpp` (C/C++)
> 
> 
> If you see a problem in this automated review, please report it here:
> http://bit.ly/2y9N9Vx
> 
> 
> ::: dom/base/nsGlobalWindowInner.cpp:4791
> (Diff revision 1)
> >    }
> >  
> >    // Get a presentation shell for use in creating the hashchange event.
> >    NS_ENSURE_STATE(IsCurrentInnerWindow());
> >  
> > -  nsIPresShell *shell = mDoc->GetShell();
> > +  RefPtr<nsPresContext> presContext = mDoc->GetPresContext();
> 
> Error: Unused "kungfudeathgrip" 'refptr<nsprescontext>' objects constructed
> from temporary values are prohibited [clang-tidy: mozilla-kungfu-death-grip]
> 
> ::: dom/base/nsGlobalWindowInner.cpp:4830
> (Diff revision 1)
> >    nsCOMPtr<nsIVariant> stateObj;
> >    rv = mDoc->GetStateObject(getter_AddRefs(stateObj));
> >    NS_ENSURE_SUCCESS(rv, rv);
> >  
> >    // Obtain a presentation shell for use in creating a popstate event.
> > -  nsIPresShell *shell = mDoc->GetShell();
> > +  RefPtr<nsPresContext> presContext = mDoc->GetPresContext();
> 
> Error: Unused "kungfudeathgrip" 'refptr<nsprescontext>' objects constructed
> from temporary values are prohibited [clang-tidy: mozilla-kungfu-death-grip]

As far as I can tell these are not kungfuDeathGrip.  I will drop them.
Comment on attachment 8952028 [details]
Bug 1417354 - Add a method to clear all pending restyle requests for a given element and its pseudos.

https://reviewboard.mozilla.org/r/221244/#review227076
Attachment #8952028 - Flags: review?(bbirtles) → review+
Comment on attachment 8952028 [details]
Bug 1417354 - Add a method to clear all pending restyle requests for a given element and its pseudos.

https://reviewboard.mozilla.org/r/221244/#review227078

::: dom/animation/EffectCompositor.h:126
(Diff revision 2)
>    // posted because updates on the main thread are throttled.
>    void PostRestyleForThrottledAnimations();
>  
> +  // Clear all pending restyle requests for the given (pseudo-) element (and its
> +  // ::before and ::after elements if the given element is not pseudo).
> +  void ClearAllPendingRestyleRequests(dom::Element* aElement);

Actually, can we just call this ClearRestyleRequestsFor() ?
Comment on attachment 8952029 [details]
Bug 1417354 - Clear pending restyle requests for the element and its pseudos where the element is detached from the document.

https://reviewboard.mozilla.org/r/221246/#review227080

I'm a bit concerned about adding three extra hashmap lookups per node when tearing down a subtree just to make a test more consistent, but I guess it only applies to nodes with animations and perhaps it saves more work in the long run if we avoid doing unnecessary restyles as a result.

::: dom/base/Element.cpp:1981
(Diff revision 2)
> +        // If the element has still animations, it's script animations, we have
> +        // to clear all pending restyle requests for the animations.

I don't understand why we would only have pending restyle requests for script animations?

DeleteProperty will cancel the animations but not the restyles, right?

If so, this comment probably needs to change (or be dropped). If we keep it, it probably should say why it's important to drop pending restyles.
Attachment #8952029 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles, travelling 20-26 Feb) from comment #12)
> ::: dom/animation/EffectCompositor.h:126
> (Diff revision 2)
> >    // posted because updates on the main thread are throttled.
> >    void PostRestyleForThrottledAnimations();
> >  
> > +  // Clear all pending restyle requests for the given (pseudo-) element (and its
> > +  // ::before and ::after elements if the given element is not pseudo).
> > +  void ClearAllPendingRestyleRequests(dom::Element* aElement);
> 
> Actually, can we just call this ClearRestyleRequestsFor() ?

Sure, will do.

(In reply to Brian Birtles (:birtles, travelling 20-26 Feb) from comment #13)
> Comment on attachment 8952029 [details]
> Bug 1417354 - Clear pending restyle requests for the element and its pseudos
> where the element is detached from the document.
> 
> https://reviewboard.mozilla.org/r/221246/#review227080
> 
> I'm a bit concerned about adding three extra hashmap lookups per node when
> tearing down a subtree just to make a test more consistent, but I guess it
> only applies to nodes with animations and perhaps it saves more work in the
> long run if we avoid doing unnecessary restyles as a result.

Yes, I think looking up hash table is cheaper than styling.

> ::: dom/base/Element.cpp:1981
> (Diff revision 2)
> > +        // If the element has still animations, it's script animations, we have
> > +        // to clear all pending restyle requests for the animations.
> 
> I don't understand why we would only have pending restyle requests for
> script animations?
> 
> DeleteProperty will cancel the animations but not the restyles, right?
> 
> If so, this comment probably needs to change (or be dropped). If we keep it,
> it probably should say why it's important to drop pending restyles.

Oh right, indeed.  I was confused between no CSS animations/transitions and no pending restyle requests.  I will drop the comment.
Comment on attachment 8952027 [details]
Bug 1417354 - Introduce nsIDocument::GetPresContext().

https://reviewboard.mozilla.org/r/221242/#review227456

::: accessible/base/nsCoreUtils.cpp:188
(Diff revision 2)
>    // EventStateManager::GetRegisteredAccessKey() method.
>    if (!aContent->IsElement() ||
>        !aContent->AsElement()->HasAttr(kNameSpaceID_None, nsGkAtoms::accesskey))
>      return 0;
>  
> -  nsIPresShell* presShell = aContent->OwnerDoc()->GetShell();
> +  nsPresContext *presContext = aContent->OwnerDoc()->GetPresContext();

nit, nsPresContext* presContext
would be the right coding style
Attachment #8952027 - Flags: review?(bugs) → review+
Comment on attachment 8952031 [details]
Bug 1417354 - Drop unused presshell and prescontext in nsGlobalWindowInner.cpp.

https://reviewboard.mozilla.org/r/221252/#review227462
Attachment #8952031 - Flags: review?(bugs) → review+
Attachment #8952029 - Attachment is obsolete: true
Attachment #8952029 - Attachment is obsolete: false
Comment on attachment 8952591 [details]
Bug 1417354 - Clear pending restyle requests for the element and its pseudos where the element is detached from the document.

https://reviewboard.mozilla.org/r/221836/#review227720

Self stamping since MozReview did not allow upload this patch with review request for Brian who does not accept any review request at this moment.  Yeah, I accidentally removed this patch from the patch set once, it confused MozReview. :/
Attachment #8952591 - Flags: review?(hikezoe) → review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d82b5673e4b6
Drop unused presshell and prescontext in nsGlobalWindowInner.cpp. r=smaug
https://hg.mozilla.org/integration/autoland/rev/3790c8adcba3
Introduce nsIDocument::GetPresContext(). r=smaug
https://hg.mozilla.org/integration/autoland/rev/86f379eb1015
Add a method to clear all pending restyle requests for a given element and its pseudos. r=birtles
https://hg.mozilla.org/integration/autoland/rev/61220454ebfb
Clear pending restyle requests for the element and its pseudos where the element is detached from the document. r=hiro
You need to log in before you can comment on or make changes to this bug.