Created attachment 552970 [details] testcase (triggers assertion after a second) ###!!! ASSERTION: wrong type: 'Type() == eEnum', file content/base/src/nsAttrValue.h, line 415 ###!!! ASSERTION: wrong type: 'BaseType() == eOtherBase', file content/base/src/nsAttrValue.h, line 507 Found by a new dom fuzzer component that modifies existing attributes.
OK - so in most cases, we don't call the attr-getters like "GetAccumulate" until after we've checked mErrorFlags to be sure that all attrs have parsed correctly. However, Jesse's found one case where we do call GetAccumulate() without having checked those flags -- in SampleAt(). This call was added in bug 617817, so I'm assuming this assertion-failure only dates back that far. We should be able to fix this by just adding a "mErrorFlags" check around that call.
Created attachment 553110 [details] [diff] [review] fix: add check for mErrorFlags
I assert that this patch won't change our rendering in any circumstances. (which is good). Explanation below. The line that I'm putting inside a conditional is just for turning on mHasChanged under certain circumstances. Now, suppose mErrorFlags is set and we now skip that line. That means our animation isn't going to participate in the animation sandwich. So the "mHasChanged" assignment doesn't make any difference in that case, anyway, since we're not going to participate in the sandwich. The one caveat (but not really) to the above is: we do need to make sure to toggle mHasChanged *when the invalid value is set*, to be sure we get a sample ASAP at that point, so that the animation's effects will be removed. That'll be handled elsewhere, though -- specifically, the attribute-setting function (e.g. SetAccumulate in this case) will set mHasChanged, so we'll be covered.
Comment on attachment 553110 [details] [diff] [review] fix: add check for mErrorFlags Looks great Daniel! Thanks for taking this!