This assertion was originally added in bug 1353208 because in that bug we
changed the type of nsSMILCompositor::mCachedBaseValue from
nsAutoPtr<nsSMILValue> to just nsSMILValue. When using nsAutoPtr,
mCachedBaseValue had two null states: one where the pointer is null, and one
where the pointed-to nsSMILValue is null. Coalescing these two states simplifies
the code but there is one case where the difference is significant as described
in the commit message for that changeset (mozilla-central changeset
ad7060dae117):
"There's a subtle difference in behavior with regards to the first sample.
Previously we would compare the (initially) null mCachedBaseValue pointer with
the passed-in nsSMILValue and set mForceCompositing to true. With this patch,
however, we will only set mForceCompositing to true if the passed-in
mCachedBaseValue is not null."
That is, if the base value we get back is a null nsSMILValue, previously we
would set mForceCompositing to true unconditionally, but with the changes in bug
1353208 we would only set that to true if the passed-in nsSMILValue was not
null.
We believed that would never matter since the passed-in nsSMILValue would never
be null if we called GetBaseValue. Quoting from that same commit message:
"... if we do call GetBaseValue the result should not be a null nsSMILValue
(except in some OOM cases where we don't really care if we miss a sample).
This patch adds an assertion to check that GetBaseValue does, in fact, return
a non-null value. (I checked the code and this appears to be the case. Even in
error cases we typically return an empty nsSMILValue of a non-null type. For
example, the early return in nsSMILCSSProperty::GetBaseValue() does this.)"
We added an assertion to validate that assumption but the crashtest included in
this patch demonstrates a case where it does not hold (specifically, when
nsStyleUtil::CSPAllowsInlineStyle returns false, nsCSSProperty::GetBaseValue
will return a null nsSMILValue).
That would seem to suggest that there is at least one case where we might fail
to set mForceIsCompositing to true and hence fail to update style on this first
sample (and presumably thereonwards too since future comparisons of
mCachedBaseValue will compare equal). However, for the case of an initial sample
mForceCompositing should already be set to true since set we update
mForceCompositing in nsSMILCompositor::GetFirstFuncToAffectSandwich() and will
make it true if *anything* in the animation function has changed and at this
point, the initial sample, *everything* will have changed. Hence, I believe
dropping this assertion is acceptable.
I have confirmed that in the crashtest in this patch, during the first sample
mForceCompositing is set to true.
I would create a reftest to test the behavior on the first sample but, at least
for the specific case where inline style is disabled due to CSP, not updating
style *is* the expected behavior so there will be no difference in behavior
regardless of whether or not the mForceCompositing flag is set.
MozReview-Commit-ID: Li0pZEH2PNl
--HG--
extra : rebase_source : a1c12a019b8481600afa4295447dc1e6fb281b22
In some circumstances it is possible to sample a timed element in the active
state with a time that precedes is current interval.
One possible sequence of steps leading to this situation is as follows:
1. A timed element (e.g. <set>, <animate>) with a non-zero begin time is the
child of <svg> element A (its "time container") but has yet to be sampled.
2. In order to resolve its initial interval, the timed element registers a
startup milestone with its time container at time 0.
3. However, before the sample is performed where the timed element's initial
current interval is resolved, <svg> element A is detached from the document
tree.
4. The timed element is then attached to a different <svg> element B that has
a current time greater than the begin time of the timed element and less than
that of <svg> element A.
5. Since the timed element is still in its startup state it registers its
startup milestone again, this time with its new time container, i.e. <svg>
element B.
6. A tick occurs or the document has its style flushed such that a sample is
performed.
This includes running the milestone sample which causes the timed element to
resolve its initial current interval. Furthermore the subsequent regular
sample of the timed element causes it to transition into its active state
because the current time of <svg> element B is greater than the begin time of
the timed element.
7. <svg> element A is re-attached to the document.
8. When we go to run the next sample, we iterate through all time containers
associated with the document's animation controller which includes both <svg>
element A, and <svg> element B.
9. <svg> element A renders up its 0 milestone from step (2) since it has yet to
run it. It converts this to parent time, i.e. the time space of the animation
controller, which will be zero or less depending on the current time of <svg>
element A when it was re-attached.
10. Since the milestone from <svg> element A will be the earliest milestone
time, it will be used as the next milestone sample time.
11. The timed element is then sampled using this time, but first it is converted
to a time in the time space of the timed element's time container, which is
now <svg> element B.
As a result of this conversion, the sample time may end up being *before*
the beginning of the timed element's current interval. Since timed elements
never expect the time to go backwards an assertion fails when it detects
that it is active, but is being sampled before its current interval.
For this particular case, ignoring the "early" sample seems to be the most
appropriate action.
More generally, however, we can anticipate other cases similar to this where
milestones are registered that cause the sample time to temporarily go
backwards. A quick audit of nsSMILTimedElement::DoSampleAt suggests that, with
the code changes from this patch, that is probably ok.
As an alternative we could, perhaps, try to drop and re-create all milestones
when time containers are re-attached to the document tree but that would add
more complexity and would not necessarily cover other similar cases of this
situation.
I have verified that the crashtest included in this changeset fails without the
code changes also in this changeset.
MozReview-Commit-ID: KKGYRayNkpo
--HG--
extra : rebase_source : 832d4b357a2a2fe07abf9eab3a6046599aff3ef5
I have verified that this fails without the code changes from the previous patch
in this series, and passes with them.
MozReview-Commit-ID: 1respvNVQaC
--HG--
extra : rebase_source : 82007792f23465edb1d286b721edeea850e2aaa3
When display style is changed from 'none' to other in animation-only restyle
we need to resolve descendant elements' style that were in the display:none
subtree.
Three possible ways to resolve the descendant elements' style;
1) Traversing unstyled elements in animation-only restyle
We can't simply traverse unstyled elements in the animation-only restyle
since when we decided to traverse the unstyled elements we don't know yet
the elements will be initially styled or are in display:none subtree. It
will result that the new elements are styled in animation-only restyle,
it's undesirable.
2) Creating a SequentialTask and resolve the descendants' style with
ServoStyleSet::StyleNewSubtree()
We can't resolve the descendants' styles with ServoStyleSet::StyleNewSubtree()
in SequentialTask since at the moment we are still in servo traversal (i.e.
sInServoTraversal is true). That means AutoSetInServoTraversal fails
in PrepareAndTraverseSubtree().
3) Creating a SequentialTask and set restyle subtree hint and defer descendants'
restyle in a subsequent normal traversal
Note that, when we process throttled animations flush, we don't process
normal traversal so the descendants will not be traversed until normal
restyle happens but it will not be a big problem since it's really rare
that user clicks display animation element just at the right moment when
display property changes from none to other. Also, if it will be really
a problem, we should process *only* transform animations on the compositor,
it's ideally right thing to do. Display property never runs on the
compositor.
This patch takes the third approach.
MozReview-Commit-ID: Krxa3kkdIq4
--HG--
extra : rebase_source : 33e9db953f21168c76716329568191625bd15896
AddOrAccumulate in nsSMILCSSValueType.cpp sets initializes |valueToAdd| to
either &valueToAddWrapper->mGeckoValue or nullptr. It then asks
FinalizeStyleAnimationValues to fill it in. FinalizeStyleAnimationValues will
return false if it could not fill it in, in which case AddOrAccumulate returns
early. As a result, after the early return we can be assured that |valueToAdd|
is not null. However, valueToAddWrapper may still be null.
Changeset 4d87f2bf4b10369af0dd83a2ef962a23299ee8d9 from bug 1358966 changed this
code such that we pass a member of valueToAddWrapper to StyleAnimationValue::Add
where we used to pass a member of valueToAdd. As a result, we can end up passing
0x20 (since valueToAddWrapper is nullptr) to Add() and then trying to read from
it.
This patch makes us pass, instead, |valueToAdd| since we know that is guaranteed
to be non-null here.
MozReview-Commit-ID: 1YwT1lBHnUe
--HG--
extra : rebase_source : abec6995af68de13eacaccf7eca7b2d121eaedf3
The two skipped tests (1 in layout/base/ and 1 in view/) both cause leaks while
running crashtests, so annotate them with a tracking bug (bug 1363000) filed.
MozReview-Commit-ID: GhSRt4wqu44
--HG--
extra : rebase_source : ed81c75251b35cee7f940b38e3998a6df3c1fcc6
Before the changes in this bug, this test used to produce an assertion:
ABORT: Rewind in the middle of a forwards seek?
This was because for a timed element whose corresponding animation element fails
conditional processing tests, we were still performing milestone samples even
though we were skipping regular samples.
Emilio Cobos Álvarez
Brian Birtles
Robert Longson
Hiroyuki Ikezoe
Bobby Holley
Yoshi Huang
Jeremy Chen
Xidorn Quan
Cameron McCormack
Sebastian Hengst
Florian Quèze
Ryan VanderMeulen
Daniel Holbert
Robert O'Callahan