1411963 - Assertion failure: !sandwichResultValue.IsNull() (Result of GetBaseValue should not be null), at /builds/worker/workspace/build/src/dom/smil/nsSMILCompositor.cpp:96

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev 64bab5cbb9b6.

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: log_minidump.txt

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: log_stderr.txt

Comment 3

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: 5175ea846fe007a1f13937a562faa1163c540e8f
INFO: First bad revision: 95e21766cd72bdda8da9bf8e8bd43909da3be861
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5175ea846fe007a1f13937a562faa1163c540e8f&tochange=95e21766cd72bdda8da9bf8e8bd43909da3be861

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

That assertion was added in bug 1353208 so it's unlikely bug 1353208 actually regressed anything functionally, but I need to work out why the assertion fails.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This is failing because in nsSMILCSSValueType::ValueFromAnimationValue we hit the check for nsStyleUtil::CSPAllowsInlineStyle and return a null value.

In the changeset that introduced the assertion I added the comment:

  "...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.)"

So clearly I missed one cases where we do, in fact, return a null value. Perhaps there are other cases too?

Provided all such cases represent exceptional error cases then perhaps it's ok if they return null. As I understand it, the consequence in that case is that if it happens as part of the initial sample, we will fail to set mForceCompositing to true in nsSMILCompositor::UpdateCachedBaseValue and, if nothing else sets it to true, we may miss fail to composite the first sample. 

However, on the first sample, surely something will set mForceCompositing to true since set we update it in nsSMILCompositor::GetFirstFuncToAffectSandwich() and will make it true if anything in the animation function has changed---at which point *everything* will have changed. So perhaps it's ok just to drop this assertion.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Bug 1411963 - Drop assertion about GetBaseValue not returning null in nsSMILCompositor::ComposeAttribute;

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.

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

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8923708 [details]
Bug 1411963 - Drop assertion about GetBaseValue not returning null in nsSMILCompositor::ComposeAttribute;

https://reviewboard.mozilla.org/r/194846/#review200182

r=me

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab9936483440
Drop assertion about GetBaseValue not returning null in nsSMILCompositor::ComposeAttribute; r=dholbert

Comment 10

[Natalia Csoregi [:nataliaCs]]

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