1245601 - Firefox nightly hangs forever on zoomcare.com

Status: VERIFIED FIXED

(Core :: DOM: Animation, defect)

Description

[Dietrich Ayala (:dietrich)]

STR:

1. Go to zoomcare.com
2. Click Urgent, then Illness/Injury
3. Click an open time, and an in-content popup appears
4. Try to click it anywhere

Firefox stops responding to all user input until force-closed and restarted. Interestingly, it doesn't show as "not responding" in Mac's Activity Monitor.

Kwierso valiantly hunted the range and narrowed it to this set of commits:

https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=cf9a5f6f14db0c2e914285a3ef36de9b8dd3cc6a&full=1

Paging Dr. Brian Birtles!

Comment 1

[Dietrich Ayala (:dietrich)]

According to his Bugzilla name, Brian is out Feb 1 - 9, so ni? Botond who reviewed the patch on bug 1228641.

Comment 2

[Nathan Froyd [:froydnj]]

Looks like that's the wrong bug number. :(

Comment 3

[Botond Ballo [:botond]]

Looks like the intended bug was bug 1232577, reviewed by :heycam.

Comment 4

[Dietrich Ayala (:dietrich)]

Oops, thanks y'all.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

FWIW, I reproduced that regression range on Windows 10, so it's not something Mac-specific. 

I didn't spot any crash reports popping up after it locked up. Maybe if I left it locked up longer it would've prompted to crash the process so we could get a crash stack?

Comment 6

[Brian Birtles (:birtles)]

I can't reproduce on a trunk debug build or Nightly on Windows 10. Cameron didn't have any success reproducing on Mac either with a debug build. I'll try building trunk after bug 1232577 and see if I can repro.

Comment 7

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles, busy 1~9 Feb) from comment #6)
> I can't reproduce on a trunk debug build or Nightly on Windows 10. Cameron
> didn't have any success reproducing on Mac either with a debug build. I'll
> try building trunk after bug 1232577 and see if I can repro.

I can't reproduce with a debug build of revision cf9a5f6f14db. Can someone who can reproduce with a debug build post a few stack traces so we can see where we might be busy. Additionally, narrowing the regression range to a specific changeset would help.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Neat, I also can't seem to reproduce it in a debug build...

It's definitely still broken for me on the latest Windows Nightly build, though.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #8)
> Neat, I also can't seem to reproduce it in a debug build...
> 
> It's definitely still broken for me on the latest Windows Nightly build,
> though.

Nightly's plugin-container.exe is constantly chewing through my CPU and memory usage has grown to 4.5GB in the time this comment was being typed.

Comment 10

[Brian Birtles (:birtles)]

(In reply to Wes Kocher (:KWierso) from comment #9)
> (In reply to Wes Kocher (:KWierso) from comment #8)
> > Neat, I also can't seem to reproduce it in a debug build...
> > 
> > It's definitely still broken for me on the latest Windows Nightly build,
> > though.
> 
> Nightly's plugin-container.exe is constantly chewing through my CPU and
> memory usage has grown to 4.5GB in the time this comment was being typed.

Interesting, if I turn e10s on, I get laggy responsiveness on Nightly but I can still close the window and it doesn't use any more CPU.

Comment 11

[Brian Birtles (:birtles)]

Actually, even with e10s I can't reproduce it. There's about a 1s hang some time after the pop-up has appeared, but that's all.

For what it's worth, the animation that seems to run when that in-content pop-up appears is a CSS animation on the visibility property.

I think the next step is to narrow down the regression to the actual changeset. From there we might be able to guess what is going on.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Are the individual changesets from 1232577 usable individually? Might be worth kicking off opt-only, build-only try pushes for each one. They should be finished in time for Dietrich or myself to try them tomorrow morning.

Comment 13

[Brian Birtles (:birtles)]

(In reply to Wes Kocher (:KWierso) from comment #12)
> Are the individual changesets from 1232577 usable individually? Might be
> worth kicking off opt-only, build-only try pushes for each one. They should
> be finished in time for Dietrich or myself to try them tomorrow morning.

Yes, they should be.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm out for the night. Ni-ing myself to kick these builds off in the morning if no one else gets to them first.

Comment 15

[Brian Birtles (:birtles)]

parent: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-ba9045466e582e11239f4dda8a276382ebd14cd1/
part 1: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-543a072b8f2c917a9ed3a1f23088eddc1d7aa95c/
part 2: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-d790ffca98a5575a53ea945c53d2f0f018a40413/
part 3: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-73ab77ec9bd332c90875c0117af0cadef8e572f7/
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-f4ea58b7c4c54d28fc0714577a0197a67310b2b7/
part 4: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-f4ea58b7c4c54d28fc0714577a0197a67310b2b7/
part 5: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-af3ff24c3b7b24d9f40e95fbc958425ec5dc9174/
part 6: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-14c6b128090f9c5e3ad8c5b09e31c25f0676c872/
part 7: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-35f4332f008d683114f63fc3a53e2c21c30d70f1/
part 8: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-d7b2a9ee3f16b4108a8805b00744d5473b5d35e0/
part 9: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-fc3dff51f1893e5c979a9bdcfc0e3423a48091bb/
part 10: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-5c7ae4edf765563cfa2a1c7b4dd821b9808f0752/
part 11: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-e37894ca33c7407655832d34a2f3f29f66ac1387/
part 12: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-b4f81c2aed078b399f3e78c1c437031a5829cde4/
part 13: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-b4f81c2aed078b399f3e78c1c437031a5829cde4/
part 14: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-570932e499c018bf2c2b89ebfc0eb74d09bf32fd/
part 15: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-772478fd8a4c5973db6556c65a9b32f6dba623c7/
part 16: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-4a76aed3166b7fefb3dcd1545ce20c61b0a7d786/
part 17: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-cf4457a970b801fd8e72378dc027fa29f538691f/
part 18: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-779c5fed138646218419e542a848183c7e030b5f/
part 19: https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-0c8af7a19ffb151513465db32b3cbf3450c944f8/

Comment 16

[Brian Birtles (:birtles)]

(Oops, accidentally put the link for part 4 there twice)

Comment 17

[Brian Birtles (:birtles)]

Looks like the Windows builds didn't work for some reason, only the OSX ones.

Comment 18

[:Gijs (he/him)]

Moving this per bug 1232577.

Comment 19

[Paul Pasca[Away. Please needinfo? Paul Oiegas [:pauloiegasSV]]]

Attachment: test.png

I've managed to reproduce this on the latest Nightly(47.0a1) on Windows 7 x32. I've also performed a regression on the same system:

45:42.22 LOG: MainThread Bisector INFO Last good revision: 26a22167872690258c715c2e45a15bfbd7898e0f
45:42.22 LOG: MainThread Bisector INFO First bad revision: cf9a5f6f14db0c2e914285a3ef36de9b8dd3cc6a

And here is the pushlog:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=26a22167872690258c715c2e45a15bfbd7898e0f&tochange=cf9a5f6f14db0c2e914285a3ef36de9b8dd3cc6a

This is reproducible even if e10s in disabled. But you need an account on zoomcare.com in order to reproduce this issue. Nightly crashes only on step 2(see attachment) when making an appointment.

Comment 20

[Dietrich Ayala (:dietrich)]

Confirmed is regression from bug 1232577.

Comment 21

[Brian Birtles (:birtles)]

It would really help if someone on Mac could tell me which changeset is responsible using the builds I posted in comment 15.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

I've done setting MacBook now.  I will try those builds now.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Done.

(In reply to Brian Birtles (:birtles) from comment #15)
> part 7:
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 35f4332f008d683114f63fc3a53e2c21c30d70f1/

https://hg.mozilla.org/integration/mozilla-inbound/rev/6511f2832185

Comment 24

[Hiroyuki Ikezoe (:hiro)]

And I've just now confirmed that patches for bug 1242872 also fixes this hangs.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Please somebody, do double-check it, you can try the build with the patches in bug 1242872 comment #80.
Thanks.

Comment 26

[Brian Birtles (:birtles)]

Thanks Hiro! I can now reproduce locally so I will try your patches later. First I want to try to debug the error a little and try to understand how it is happening.

Comment 27

[Brian Birtles (:birtles)]

Bisecting locally, part 7 seems fine. I think it's part 8 that introduces the issue.

https://hg.mozilla.org/mozilla-central/rev/e3aa66edbd85

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Sorry for the mistake. part 7 must be the last good revision.

Comment 29

[Brian Birtles (:birtles)]

Now that I can reproduce this locally I think I isolated the offending stack:

> xul.dll!mozilla::dom::KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() Line 194	C++
> xul.dll!mozilla::dom::Animation::UpdateEffect() Line 1035	C++
> xul.dll!mozilla::dom::Animation::UpdateTiming(mozilla::dom::Animation::SeekFlag aSeekFlag, mozilla::dom::Animation::SyncNotifyFlag aSyncNotifyFlag) Line 972	C++
> xul.dll!mozilla::dom::CSSAnimation::UpdateTiming(mozilla::dom::Animation::SeekFlag aSeekFlag, mozilla::dom::Animation::SyncNotifyFlag aSyncNotifyFlag) Line 301	C++
> xul.dll!mozilla::dom::Animation::DoPlay(mozilla::ErrorResult & aRv, mozilla::dom::Animation::LimitBehavior aLimitBehavior) Line 856	C++
> xul.dll!mozilla::dom::CSSAnimation::PlayFromStyle() Line 90	C++
> xul.dll!nsAnimationManager::BuildAnimations(nsStyleContext * aStyleContext, mozilla::dom::Element * aTarget, mozilla::dom::AnimationTimeline * aTimeline, nsTArray<RefPtr<mozilla::dom::Animation> > & aAnimations) Line 704	C++
> xul.dll!nsAnimationManager::CheckAnimationRule(nsStyleContext * aStyleContext, mozilla::dom::Element * aElement) Line 410	C++
> xul.dll!nsStyleSet::GetContext(nsStyleContext * aParentContext, nsRuleNode * aRuleNode, nsRuleNode * aVisitedRuleNode, nsIAtom * aPseudoTag, nsCSSPseudoElements::Type aPseudoType, mozilla::dom::Element * aElementForAnimation, unsigned int aFlags) Line 977	C++
> xul.dll!nsStyleSet::ResolveStyleWithReplacement(mozilla::dom::Element * aElement, mozilla::dom::Element * aPseudoElement, nsStyleContext * aNewParentContext, nsStyleContext * aOldStyleContext, nsRestyleHint aReplacements, unsigned int aFlags) Line 1729	C++
> xul.dll!mozilla::ElementRestyler::RestyleSelf(nsIFrame * aSelf, nsRestyleHint aRestyleHint, unsigned int * aSwappedStructs, nsTArray<mozilla::ElementRestyler::SwapInstruction> & aSwaps) Line 3945	C++
> xul.dll!mozilla::ElementRestyler::Restyle(nsRestyleHint aRestyleHint) Line 3280	C++
> xul.dll!mozilla::ElementRestyler::ComputeStyleChangeFor(nsIFrame * aFrame, nsStyleChangeList * aChangeList, nsChangeHint aMinChange, mozilla::RestyleTracker & aRestyleTracker, nsRestyleHint aRestyleHint, const mozilla::RestyleHintData & aRestyleHintData, nsTArray<mozilla::ElementRestyler::ContextToClear> & aContextsToClear, nsTArray<RefPtr<nsStyleContext> > & aSwappedStructOwners) Line 4522	C++
> xul.dll!mozilla::RestyleManager::ComputeAndProcessStyleChange(nsIFrame * aFrame, nsChangeHint aMinChange, mozilla::RestyleTracker & aRestyleTracker, nsRestyleHint aRestyleHint, const mozilla::RestyleHintData & aRestyleHintData) Line 4933	C++
> xul.dll!mozilla::RestyleManager::RestyleElement(mozilla::dom::Element * aElement, nsIFrame * aPrimaryFrame, nsChangeHint aMinHint, mozilla::RestyleTracker & aRestyleTracker, nsRestyleHint aRestyleHint, const mozilla::RestyleHintData & aRestyleHintData) Line 1055	C++
> xul.dll!mozilla::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element * aElement, nsRestyleHint aRestyleHint, nsChangeHint aChangeHint, const mozilla::RestyleHintData & aRestyleHintData) Line 196	C++
> xul.dll!mozilla::RestyleTracker::DoProcessRestyles() Line 354	C++
> xul.dll!mozilla::RestyleManager::ProcessRestyles(mozilla::RestyleTracker & aRestyleTracker) Line 533	C++
> xul.dll!mozilla::RestyleManager::ProcessPendingRestyles() Line 1776	C++
> xul.dll!PresShell::FlushPendingNotifications(mozilla::ChangesToFlush aFlush) Line 3984	C++
> xul.dll!PresShell::FlushPendingNotifications(mozFlushType aType) Line 3865	C++
> xul.dll!nsDocument::FlushPendingNotifications(mozFlushType aType) Line 8164	C++
> xul.dll!nsComputedDOMStyle::UpdateCurrentStyleSources(bool aNeedsLayoutFlush) Line 642	C++
> xul.dll!nsComputedDOMStyle::GetPropertyCSSValue(const nsAString_internal & aPropertyName, mozilla::ErrorResult & aRv) Line 804	C++
> xul.dll!nsComputedDOMStyle::GetPropertyValue(const nsAString_internal & aPropertyName, nsAString_internal & aReturn) Line 383	C++
> xul.dll!nsComputedDOMStyle::GetPropertyValue(const nsCSSProperty aPropID, nsAString_internal & aValue) Line 318	C++
> xul.dll!nsDOMCSSDeclaration::GetTransitionDuration(nsAString_internal & aValue, mozilla::ErrorResult & rv) Line 3548	C++
> xul.dll!mozilla::dom::CSS2PropertiesBinding::get_transitionDuration(JSContext * cx, JS::Handle<JSObject *> obj, nsDOMCSSDeclaration * self, JSJitGetterCallArgs args) Line 31312	C++
> xul.dll!mozilla::dom::GenericBindingGetter(JSContext * cx, unsigned int argc, JS::Value * vp) Line 2648	C++
> xul.dll!js::CallJSNative(JSContext * cx, bool (JSContext *, unsigned int, JS::Value *) * native, const JS::CallArgs & args) Line 235	C++
> xul.dll!js::Invoke(JSContext * cx, const JS::CallArgs & args, js::MaybeConstruct construct) Line 479	C++
> xul.dll!js::Invoke(JSContext * cx, const JS::Value & thisv, const JS::Value & fval, unsigned int argc, const JS::Value * argv, JS::MutableHandle<JS::Value> rval) Line 531	C++
> xul.dll!js::InvokeGetter(JSContext * cx, const JS::Value & thisv, JS::Value fval, JS::MutableHandle<JS::Value> rval) Line 640	C++
> xul.dll!CallGetter(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<JS::Value> receiver, JS::Handle<js::Shape *> shape, JS::MutableHandle<JS::Value> vp) Line 1672	C++
> xul.dll!GetExistingProperty<1>(JSContext * cx, JS::Handle<JS::Value> receiver, JS::Handle<js::NativeObject *> obj, JS::Handle<js::Shape *> shape, JS::MutableHandle<JS::Value> vp) Line 1724	C++
> xul.dll!NativeGetPropertyInline<1>(JSContext * cx, JS::Handle<js::NativeObject *> obj, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, IsNameLookup nameLookup, JS::MutableHandle<JS::Value> vp) Line 1939	C++
> xul.dll!js::NativeGetProperty(JSContext * cx, JS::Handle<js::NativeObject *> obj, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 1973	C++
> xul.dll!js::GetProperty(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 1471	C++
> xul.dll!JS_ForwardGetPropertyTo(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<jsid> id, JS::Handle<JS::Value> receiver, JS::MutableHandle<JS::Value> vp) Line 2544	C++
> xul.dll!mozilla::dom::GetPropertyOnPrototype(JSContext * cx, JS::Handle<JSObject *> proxy, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, bool * found, JS::MutableHandle<JS::Value> vp) Line 1801	C++
> xul.dll!mozilla::dom::CSS2PropertiesBinding::DOMProxyHandler::get(JSContext * cx, JS::Handle<JSObject *> proxy, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 44057	C++
> xul.dll!js::Proxy::get(JSContext * cx, JS::Handle<JSObject *> proxy, JS::Handle<JS::Value> receiver_, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 300	C++
> xul.dll!js::proxy_GetProperty(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 572	C++
> xul.dll!js::GetProperty(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<JS::Value> receiver, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 1470	C++
> xul.dll!js::GetProperty(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<JSObject *> receiver, JS::Handle<jsid> id, JS::MutableHandle<JS::Value> vp) Line 831	C++
> xul.dll!js::GetObjectElementOperation(JSContext * cx, JSOp op, JS::Handle<JSObject *> obj, JS::Handle<JSObject *> receiver, JS::Handle<JS::Value> key, JS::MutableHandle<JS::Value> res) Line 449	C++
> xul.dll!js::GetElementOperation(JSContext * cx, JSOp op, JS::MutableHandle<JS::Value> lref, JS::Handle<JS::Value> rref, JS::MutableHandle<JS::Value> res) Line 554	C++
> xul.dll!js::jit::DoGetElemFallback(JSContext * cx, js::jit::BaselineFrame * frame, js::jit::ICGetElem_Fallback * stub_, JS::Handle<JS::Value> lhs, JS::Handle<JS::Value> rhs, JS::MutableHandle<JS::Value> res) Line 1804	C++

Comment 30

[Brian Birtles (:birtles)]

Basically, we're triggering restyles while building animations. That's why bug 1242872 fixes this.

I've often wondered about how we can avoid this problem. We have a bunch of methods on Animation and so on where there is the variant we call doing style updates that doesn't trigger further restyles, and the variant we call from script etc. which does trigger further restyles.

I think we could probably add some sort of stack-based class that sets a flag on the EffectCompositor (perhaps a class static flag?) that will cause us to ignore all calls to RequestRestyle. That would potentially allow us to use the same methods from either script or style.

It's possible that might even solve some of the other bugs we have on file in this area.

We might need a more simple fix for 46, however which is the first version where this regression shows up.

Comment 31

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #29)
> Now that I can reproduce this locally I think I isolated the offending stack:
> 
> > xul.dll!mozilla::dom::KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() Line 194	C++
> > xul.dll!mozilla::dom::Animation::UpdateEffect() Line 1035	C++

Oops! Missed the very top of the stack!

> xul.dll!mozilla::EffectCompositor::RequestRestyle(mozilla::dom::Element * aElement, nsCSSPseudoElements::Type aPseudoType, mozilla::EffectCompositor::RestyleType aRestyleType, mozilla::EffectCompositor::CascadeLevel aCascadeLevel) Line 153	C++
> xul.dll!mozilla::AnimationCollection::RequestRestyle(mozilla::EffectCompositor::RestyleType aRestyleType) Line 488	C++
> xul.dll!mozilla::dom::KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() Line 194	C++

Comment 32

[Brian Birtles (:birtles)]

Stack line numbers are from revision https://hg.mozilla.org/mozilla-central/rev/e3aa66edbd85

I don't quite understand why this doesn't happen all the time. I also note that we've seen similar bugs that only crop up when both animations and downloadable fonts are in use.

Comment 33

[Brian Birtles (:birtles)]

I'll look into this tomorrow.

Comment 34

[Brian Birtles (:birtles)]

Attachment: Proof of concept fix

After a little bit of investigation so far it seems like:

* We end up with a situation where we perpetually post restyles.

* At least part of the cause is that when we go to refresh animations
  we rebuild the list of animations by creating new animations and calling
  Play() and other methods on them which trigger animation restyles to be posted.

* Normally methods like Play() etc. *should* post restyles even when called
  while building animations but the problem is we are doing this even when
  there is no change to the animations. i.e. we build up the list of animations,
  compare it to the existing animations and discover nothing has changed but in
  the mean time we've posted a needless animation restyle.

* Prior to bug 1232577 two things were different:

  - We didn't trigger the restyles from within Play() etc. but from the point
    where we build animations. I'm not sure if this is significant.
  - We kept a timestamp to record if the animation rule was up to date (and
    manually cleared it when we had reason to think we needed to update the
    animation rule twice within a tick). That was quite a blunt tool and by
    getting rid of it we got a significant performance boost, but it probably
    stopped this particular situation from occurring.

* Nevertheless, even with the current arrangement we don't normally end up
  doing restyles endlessly, perhaps because of the particular restyle hint we
  end up posting? However, in some cases such as this bug, we do. I don't really
  know what the trigger is but downloadable fonts seems to be a common
  factor between this bug and bug 1247914. It seems like the restyles we trigger
  here are being picked up by something else which is then subsequently triggers
  a restyle that brings us back to refresh the set of animations.

* The first obvious remedy is to not end up triggering restyles when nothing
  changes. Bug 1242872 (which just landed on mozilla-inbound today) should fix
  that but is probably too large to uplift to Aurora.

* The second step which *may* help to avoid similar bugs (and especially bug
  1247914) is to not trigger resolving styles within building animations like
  we currently do. Bug 1245748 will make us store specified styles in keyframes
  so we possibly won't need to resolve computed style until a later point.
  That bug too, apart from not yet being fixed, will be too big to uplift
  to Aurora.

* The issue is what to do with Firefox 46. Attached is a proof of concept fix.
  It simply makes the EffectCompositor ignore calls to RequestRestyle while
  we're building animations and then tries to detect after-the-fact if anything
  changed so we can post the restyle in that case. It appears to fix zoomcare
  and it passes the subset of animation tests I ran but it's not pretty.

I'll have a bit more of a think about it and Hiro said he'd look into it tomorrow but for now I'm wondering, David, if any of this makes sense? Or do you have any other suggestions about what we should be doing here?

Comment 35

[Hiroyuki Ikezoe (:hiro)]

I am going to note what I know.

(In reply to Brian Birtles (:birtles) from comment #34)
> * Nevertheless, even with the current arrangement we don't normally end up
>   doing restyles endlessly, perhaps because of the particular restyle hint we
>   end up posting? However, in some cases such as this bug, we do. I don't
> really
>   know what the trigger is but downloadable fonts seems to be a common
>   factor between this bug and bug 1247914. 

And bug 1245260 too.

> * The first obvious remedy is to not end up triggering restyles when nothing
>   changes. Bug 1242872 (which just landed on mozilla-inbound today) should
> fix
>   that but is probably too large to uplift to Aurora.

Yes, I've just confirmed now that bug 1242872 fixes this bug too, but does not 
fix bug 1245260 because it's a CSS transitions case I guess. 
And sadly I've just also confirmed that bug 1245260 happens on Firefox 46 too.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Another possible fix is that each CSSAnimation just stores its playState (i.e. GetPlayState()) in BuildAnimations (don't play or pause in BuildAnimations!) and the each CSSAnimation is played or paused in response to the playState in CheckAnimationRule only if the animation is newly created.  An advantage of this approach is that this is a subset of part 7 patch for bug 1242872.  Though, with this approach, part 6 patch for bug 1242872 has to be also uplifted, it's relatively tiny fix.

Comment 37

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> Another possible fix is that each CSSAnimation just stores its playState
> (i.e. GetPlayState()) in BuildAnimations (don't play or pause in
> BuildAnimations!) and the each CSSAnimation is played or paused in response
> to the playState in CheckAnimationRule only if the animation is newly
> created.  An advantage of this approach is that this is a subset of part 7
> patch for bug 1242872.  Though, with this approach, part 6 patch for bug
> 1242872 has to be also uplifted, it's relatively tiny fix.

Hiro, do you want to make up a patch for 46 that just includes the subset of part 7 and part 6 we need to fix this?

Comment 38

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #37)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #36)
> > Another possible fix is that each CSSAnimation just stores its playState
> > (i.e. GetPlayState()) in BuildAnimations (don't play or pause in
> > BuildAnimations!) and the each CSSAnimation is played or paused in response
> > to the playState in CheckAnimationRule only if the animation is newly
> > created.  An advantage of this approach is that this is a subset of part 7
> > patch for bug 1242872.  Though, with this approach, part 6 patch for bug
> > 1242872 has to be also uplifted, it's relatively tiny fix.
> 
> Hiro, do you want to make up a patch for 46 that just includes the subset of
> part 7 and part 6 we need to fix this?

Sure.  I had a halfway patch for that actually, but can not find it now..  Anyway I can recreate it soon.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Attachment: A possible fix for firefox 46

This is the patch.
I just checked that this patch solves zoomcare.com issue and all tests under dom/animation/test and layout/style/test/test_animation passed.
How can we test patches against mozilla-aurora on try server?

Comment 40

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df1ba0fdab80

Comment 41

[Hiroyuki Ikezoe (:hiro)]

The try result seems good. Though there are some brand new oranges <https://treeherder.mozilla.org/logviewer.html#?job_id=17511567&repo=try>, I believe the oranges are not related to the patch at all.  Sheriff, it's worth filing a new intermittent? Or is it known failures on mozilla-aurora? philor?

Brian, do you mind reviewing the patch? Or should we wait for the view from dbaron?

Comment 42

[Hiroyuki Ikezoe (:hiro)]

I should note about automation test for this issue.  I did try to write it before but have not succeeded yet.

Comment 43

[Phil Ringnalda (:philor)]

Try looks fine to me - beta can be difficult, but aurora usually works as long as you know one trick: when a result looks odd to you, first check aurora itself to see if we're running that suite there. That took care of all your brand new looking Windows e10s devtools and web-platform failures, we aren't actually running those on aurora. I'm mildly concerned by the Win7 bc2 and dt4 leaks, but that has nothing to do with your patch and everything to do with the way we are apparently using AWS VM to run Win7 tests only on try, where you are the only person who will actually look at the results and have a chance of noticing that they do not actually work, since sheriffs don't really look at try and most developers will neither look nor say anything.

Comment 44

[Brian Birtles (:birtles)]

Comment on attachment 8725937 [details] [diff] [review]
A possible fix for firefox 46

Review of attachment 8725937 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is better than my patch. It's late in the cycle and this patch at least represents a subset of something that's had bake time on m-c so I think it's safer to go with this.

Please request approval to land on aurora.

::: layout/style/nsAnimationManager.cpp
@@ +635,5 @@
>  
> +    // Do not play or pause for this temporary animation here because
> +    // if and only if the temporary animation is not for a new animation,
> +    // i.e. there is another animation which has the same style name,
> +    // to play or pause causes infinite restyling on some web sites.

Do not play or pause this temporary animation here. We will
*actually* play or pause it if it doesn't match any existing
animation. If it matches an existing animation we will read this
initial state back and use it to determine if we need to update
the existing animation's state.

This allows us to avoid requesting unnecessary restyles (which
would be triggered if we call Play()/Pause()) while updating
animations, particularly in the case where nothing has changed.

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Attachment: Avoid requesting unnecessary restyles

Approval Request Comment
[Feature/regressing bug #]: bug 1232577
[User impact if declined]: browser hangs
[Describe test coverage new/current, TreeHerder]: Tested manually on the problematic site for this particular issue and all current tests passed on try
[Risks and why]: Relatively low: This is a small subset of patches for bug 1247929 which has been landed in mozilla-central for a week.
[String/UUID change made/needed]: None

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

This isn't marked fixed for 47, Wes, did all of the work land correctly?

Comment 47

[Liz Henry (:lizzard) (relman/hg->git project)]

re: comment 43, I am not sure that the tests not being enabled yet on aurora should be a comfort!  But we can give this a shot for aurora, if someone could confirm that what's in m-c actually fixes the reported issue.  

It will also help reassure me about uplift if you can keep an eye out for regressions, and be ready to back out this work if we see more than a couple of (quickly fixable) regressions.

Comment 48

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8726072 [details] [diff] [review]
Avoid requesting unnecessary restyles

Let's try it. Hangs are bad!
But, since there are some test failures with e10s and dev tools, we should watch for possible problems.

Comment 49

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #45)
> Created attachment 8726072 [details] [diff] [review]
> Avoid requesting unnecessary restyles
> 
> Approval Request Comment
> [Feature/regressing bug #]: bug 1232577
> [User impact if declined]: browser hangs
> [Describe test coverage new/current, TreeHerder]: Tested manually on the
> problematic site for this particular issue and all current tests passed on
> try
> [Risks and why]: Relatively low: This is a small subset of patches for bug
> 1247929 which has been landed in mozilla-central for a week.

I am sorry, I pasted a wrong bug number here.  The correct is bug 1242872 which fixed this issue in mozilla-central.

Comment 50

[Phil Ringnalda (:philor)]

Those devtools failures have nothing to do with the patch here, they are instead the reason we are not yet running devtools tests on aurora.

I guess this is status-firefox47: fixed by virtue of being fixed by another bug, though it would be as reasonable to untrack and call this unaffected for 47 and track bug 1242872 transitively.

Comment 51

[Liz Henry (:lizzard) (relman/hg->git project)]

OK. So, does the patch in this bug need uplift to 46, or not?  
I'll track 1242872.

Comment 52

[Brian Birtles (:birtles)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #51)
> OK. So, does the patch in this bug need uplift to 46, or not?  
> I'll track 1242872.

Yes this needs uplift. Bug 1242872 fixes this for 47 but is too big to uplift to 46. The patch here is just a subset of the approach in bug 1242872.

Comment 53

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks. I appreciate the smaller and hopefully less risky uplift patch! This should land for either Friday's or Saturday's aurora, then.  After that, we can verify the fix either on aurora or in 46 beta 1 next week.

Comment 54

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc69b98ccbdb

Comment 55

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

This bug is verified fixed on:
- 46.0b6-build1 (20160328182534),
- 47.0a2 (2016-03-30),
- 48.0a1 (2016-03-30),
using Ubuntu 14.04 x86, Mac OS X 10.9.5 and Windows 10 x64.