1374966 - stylo: ASSERTION: Unexpected UpdateTransformLayer hint while running test_transitions_per_property.html

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Chiou [:boris]]

Assertion while running test_transitions_per_property.html

GECKO(14014) | [Child 14020] ###!!! ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle()', file /Users/boris/projects/firefox/gecko/layout/base/RestyleManager.cpp, line 1187
GECKO(14014) | #01: mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::TraversalRestyleBehavior) (nsTArray.h:398, in XUL)
GECKO(14014) | #02: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) (ServoRestyleManager.cpp:702, in XUL)
GECKO(14014) | #03: nsDocument::FlushPendingNotifications(mozilla::FlushType) (nsDocument.h:1231, in XUL)
GECKO(14014) | #04: nsComputedDOMStyle::UpdateCurrentStyleSources(bool) (nsComputedDOMStyle.cpp:814, in XUL)
GECKO(14014) | #05: nsComputedDOMStyle::GetPropertyCSSValue(nsAString const&, mozilla::ErrorResult&) (nsComputedDOMStyle.cpp:1001, in XUL)
GECKO(14014) | #06: nsComputedDOMStyle::GetPropertyValue(nsAString const&, nsAString&) (AlreadyAddRefed.h:121, in XUL)
GECKO(14014) | #07: mozilla::dom::CSSStyleDeclarationBinding::getPropertyValue(JSContext*, JS::Handle<JSObject*>, nsICSSDeclaration*, JSJitMethodCallArgs const&) (nsICSSDeclaration.h:129, in XUL)
GECKO(14014) | #08: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (BindingUtils.cpp:2962, in XUL)
GECKO(14014) | #09: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:294, in XUL)
GECKO(14014) | #10: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:470, in XUL)
GECKO(14014) | #11: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:3067, in XUL)
GECKO(14014) | #12: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:410, in XUL)
GECKO(14014) | #13: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:488, in XUL)
GECKO(14014) | #14: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:3067, in XUL)
GECKO(14014) | #15: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:410, in XUL)
GECKO(14014) | #16: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:488, in XUL)
GECKO(14014) | #17: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (Interpreter.cpp:534, in XUL)
GECKO(14014) | #18: PromiseReactionJob(JSContext*, unsigned int, JS::Value*) (Promise.cpp:1001, in XUL)
GECKO(14014) | #19: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:294, in XUL)
GECKO(14014) | #20: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:470, in XUL)
GECKO(14014) | #21: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (Interpreter.cpp:534, in XUL)
GECKO(14014) | #22: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (jsapi.cpp:2948, in XUL)
GECKO(14014) | #23: mozilla::dom::PromiseJobCallback::Call(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&) (PromiseBinding.cpp:21, in XUL)
GECKO(14014) | #24: mozilla::dom::PromiseJobCallback::Call(mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) (PromiseBinding.h:89, in XUL)
GECKO(14014) | #25: mozilla::PromiseJobRunnable::Run() (ErrorResult.h:600, in XUL)
GECKO(14014) | #26: mozilla::dom::Promise::PerformMicroTaskCheckpoint() (Promise.cpp:537, in XUL)
GECKO(14014) | #27: nsGlobalWindow::RunTimeoutHandler(mozilla::dom::Timeout*, nsIScriptContext*) (nsGlobalWindow.cpp:13307, in XUL)
GECKO(14014) | #28: mozilla::dom::TimeoutManager::RunTimeout(mozilla::TimeStamp const&, mozilla::TimeStamp const&) (TimeoutManager.cpp:735, in XUL)
GECKO(14014) | #29: mozilla::dom::TimeoutExecutor::MaybeExecute() (TimeoutExecutor.cpp:168, in XUL)
GECKO(14014) | #30: mozilla::dom::TimeoutExecutor::Run() (TimeoutExecutor.cpp:224, in XUL)
GECKO(14014) | #31: mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() (ThrottledEventQueue.cpp:195, in XUL)
GECKO(14014) | #32: mozilla::ThrottledEventQueue::Inner::Executor::Run() (ThrottledEventQueue.cpp:75, in XUL)
GECKO(14014) | #33: mozilla::SchedulerGroup::Runnable::Run() (SchedulerGroup.cpp:368, in XUL)
GECKO(14014) | #34: nsThread::ProcessNextEvent(bool, bool*) (Maybe.h:224, in XUL)
GECKO(14014) | #35: NS_ProcessPendingEvents(nsIThread*, unsigned int) (nsThreadUtils.cpp:419, in XUL)
GECKO(14014) | #36: nsBaseAppShell::NativeEventCallback() (nsBaseAppShell.cpp:98, in XUL)
GECKO(14014) | #37: nsAppShell::ProcessGeckoEvents(void*) (nsAppShell.mm:400, in XUL)
GECKO(14014) | #38: CFStringFold (in CoreFoundation) + 4001
GECKO(14014) | #39: -[_CFXNotificationRegistrar match:object:observer:enumerator:] (in CoreFoundation) + 1165
GECKO(14014) | #40: CFCharacterSetIsCharacterMember (in CoreFoundation) + 470
GECKO(14014) | #41: CFStringCapitalize (in CoreFoundation) + 1780
GECKO(14014) | #42: RunCurrentEventLoopInMode (in HIToolbox) + 240
GECKO(14014) | #43: ReceiveNextEventCommon (in HIToolbox) + 432
GECKO(14014) | #44: _BlockUntilNextEventMatchingListInModeWithFilter (in HIToolbox) + 71
GECKO(14014) | #45: _DPSNextEvent (in AppKit) + 1120
GECKO(14014) | #46: -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] (in AppKit) + 2796
GECKO(14014) | #47: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (nsAppShell.mm:130, in XUL)
GECKO(14014) | #48: -[NSApplication run] (in AppKit) + 926
GECKO(14014) | #49: nsAppShell::Run() (nsCOMPtr.h:551, in XUL)
GECKO(14014) | #50: XRE_RunAppShell() (nsEmbedFunctions.cpp:896, in XUL)
GECKO(14014) | #51: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (MessagePump.cpp:269, in XUL)
GECKO(14014) | #52: MessageLoop::Run() (message_loop.cc:587, in XUL)
GECKO(14014) | #53: XRE_InitChildProcess(int, char**, XREChildData const*) (nsEmbedFunctions.cpp:716, in XUL)
GECKO(14014) | #54: main (plugin-container.cpp:64, in plugin-container)

Comment 1

[Darkspirit]

I saw this in a debug try build in bug 1381431 comment 5 while testing bug 1378064.

Comment 2

[Brian Birtles (:birtles)]

Attachment: Minimized version of test_transitions_per_property.html

test_transitions_per_property.html is a very busy test file but after a couple of hours I managed to reduce it to something fairly simple that reproduces this assertion.

It looks like this:

  const cs = getComputedStyle(div);
  const winUtils = SpecialPowers.getDOMWindowUtils(window);

  winUtils.advanceTimeAndRefresh(0);

  div.style.transitionProperty = 'transform';
  div.style.transform = 'rotate(60deg)';
  cs.transform;
  await waitForPaints();

  winUtils.advanceTimeAndRefresh(200000);

  div.style.transitionProperty = 'none';
  div.style.transform = 'none';
  cs.transform;

I haven't analyzed it any further than this, however.

(The fact that this took about 2 hours to reduce this test is, I hope, a useful data point when it comes to arguing for smaller, simpler, self-contained tests. The smil-grid.js reftests are also quite bad like this.)

Comment 3

[Brian Birtles (:birtles)]

Note that attachment 8889300 [details] won't reproduce the assertion when loaded by itself. You'll need to overwrite test_transitions_per_property.html or some other style mochitest in order to run it since it needs SpecialPowers and paint listener.js.

Comment 4

[Brian Birtles (:birtles)]

So, just at a glance, it's significant that the assertion doesn't fire without the final steps where we drop both transition, the transform, and force a style flush.

Furthermore, the assertion we're failing is:

  NS_ASSERTION(!(aChange & nsChangeHint_UpdateTransformLayer) ||
               aFrame->IsTransformed() ||
               aFrame->StyleDisplay()->HasTransformStyle(),
               "Unexpected UpdateTransformLayer hint");

That, I suppose, will cause the IsTransform() and HasTransformStyle() checks to return false. So then the question is, should we still get the UpdateTransformLayer change hint? Layers will need to be updated but would this happen without the hint? Or is the assertion invalid? And how does Gecko work here?

Comment 5

[Brian Birtles (:birtles)]

I haven't had a deep look into this, but so far what I can see is that we have the following sequence:

(at the end of the test)
* Clear transition
* Clear transform
* Trigger style flush
  - Inside finish_restyle we call accumulate_damage_for
    - From there we call compute_style_difference
      As part of that we call into nsStyleDisplay::CalcDifference where we have
      the following check:

        if (!mSpecifiedTransform != !aNewData.mSpecifiedTransform ||
            (mSpecifiedTransform &&
            *mSpecifiedTransform != *aNewData.mSpecifiedTransform)) {
          transformHint |= nsChangeHint_UpdateTransformLayer;
          ...

    We end up writing that restyle damage with the
    nsChangeHint_UpdateTransformLayer change hint back to the ElementData passed
    into finish_restyle.

  - Once we get inside ProcessPostTraversal we call Servo_TakeChangeHint and get
    back the change hint including nsChangeHint_UpdateTransformLayer.

    Note that it's Servo_TakeChangeHint that gives us this change hint, NOT
    AddLayerChangesForAnimation (which is very careful *not* to set this hint in
    order to avoid triggering this assertion).

Next I need to work out why we don't end up getting this hint in Gecko.

Comment 6

[Brian Birtles (:birtles)]

After looking into Gecko, I discovered Gecko also sets this hint. The difference is that Gecko processes change hints from animation restyles at the end of the animation restyle and before doing the regular restyle.

We have the following situation:

* Run animation-restyle
  * Notice that the transform has changed (due to the advanceTimeAndRefresh(200000) in this case)
  * Produce an nsChangeHint_UpdateTransformLayer hint
* Run regular restyle
  * Notice that there is now no transform at all
  * Produce an nsChangeHint_AddOrRemoveTransform (and RepaintFrame etc.)

In Stylo we'll actually accumulate both hints together and then process them after removing the transform style so when we call into ApplyRenderingChangeToTree we'll trip up on the assertion that expects that we only set nsChangeHint_UpdateTransformLayer when we have transform style.

I suspect the correct fix here is just to drop nsChangeHint_UpdateTransformLayer when we already have nsChangeHint_AddOrRemoveTransform:

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

Comment 7

[Brian Birtles (:birtles)]

Attachment: Bug 1374966 - Drop nsChangeHint_UpdateTransformLayer when we also have nsChangeHint_AddOrRemoveTransform;

Review commit: https://reviewboard.mozilla.org/r/161828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/161828/

Comment 8

[Cameron McCormack (:heycam)]

I'm not terribly familiar with the transform layer related handling, so a question: is it possible that we generate both UpdateTransformLayer and AddOrRemoveTransform (from an animation and normal restyle in stylo), when we're changing from no transform to some transform?  I guess not, otherwise we would be asserting whenever we start a transition that adds a new transform.

Would it make sense to assert, when we do decide to remove the UpdateTransformLayer hint, that the frame has no transform style?

Comment 9

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #8)
> I'm not terribly familiar with the transform layer related handling, so a
> question: is it possible that we generate both UpdateTransformLayer and
> AddOrRemoveTransform (from an animation and normal restyle in stylo), when
> we're changing from no transform to some transform?  I guess not, otherwise
> we would be asserting whenever we start a transition that adds a new
> transform.
> 
> Would it make sense to assert, when we do decide to remove the
> UpdateTransformLayer hint, that the frame has no transform style?

I thought about adding that but I decided it wasn't necessary. The two hints are supposed to orthogonal. We only add AddOrRemoveTransform in [1] and there we either add that or UpdateTransformLayer but never both. Elsewhere we make sure not to add both at the same time.[2] Conceptually we're either adding or removing a transform, or we're updating one. If we update then remove, then that's equivalent to just removing (the case in this bug) and if we add then update, then that's equivalent to just adding since presumably between the AddOrRemoveTransform hint and any other hints we add at the same time (e.g. RepaintFrame) we're triggering all the work we need to do when we go from no transform to some transform.

Or at least that was my reasoning when I first went to add different assertions here.

[1] http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/layout/style/nsStyleStruct.cpp#3599
[2] http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/layout/base/RestyleManager.cpp#1750-1761

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8890657 [details]
Bug 1374966 - Drop nsChangeHint_UpdateTransformLayer when we also have nsChangeHint_AddOrRemoveTransform;

https://reviewboard.mozilla.org/r/161828/#review167636

OK, thanks.  I think this patch is fine as is then.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d372311a32e
Drop nsChangeHint_UpdateTransformLayer when we also have nsChangeHint_AddOrRemoveTransform; r=heycam

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d372311a32e