Bug 91623 – Possible null pointer dereferences in AnimationController.cpp

• Component: WebCore Misc.
• Status: RESOLVED CONFIGURATION CHANGED

[W. James MacLean]

Possible null pointer dereferences in AnimationController.cpp

[W. James MacLean]

Created attachment 153008 [details]
Patch

[W. James MacLean]

Looking for guidance as to what tests would be appropriate here, or do we even need any for something as basic as guarding against a null-pointer dereference in a location where null pointers seem to be allowed?

[Alexey Proskuryakov]

> do we even need any for something as basic as guarding against a null-pointer dereference in a location where null pointers seem to be allowed?

Yes. There are many reasons to have regression tests besides proving that a fix is correct:

- prevent playing whack-a-mole game of fixing the same bug over and over again;
- make it possible to verify that the fix is present in a built product (especially useful when merging fixes to a different branch);
- provide testing coverage for other related code paths in the same apparently poorly tested area (taking this patch as an example, you fix two null dereferences, but perhaps any code that could trigger these crashes some place later, so you don't improve behavior for end users at all?)
- help assess severity of a bug (if the code is doing something sensible, shipping a fix in a release can be expedited as compared to a completely useless sequence of calls that happen to trigger a crash).

In cases other than crashes, a regression test is also useful for comparing behavior with other browsers.

[W. James MacLean]

(In reply to comment #3)
> > do we even need any for something as basic as guarding against a null-pointer dereference in a location where null pointers seem to be allowed?
> 
> Yes. There are many reasons to have regression tests besides proving that a fix is correct:
> 
> - prevent playing whack-a-mole game of fixing the same bug over and over again;
> - make it possible to verify that the fix is present in a built product (especially useful when merging fixes to a different branch);
> - provide testing coverage for other related code paths in the same apparently poorly tested area (taking this patch as an example, you fix two null dereferences, but perhaps any code that could trigger these crashes some place later, so you don't improve behavior for end users at all?)
> - help assess severity of a bug (if the code is doing something sensible, shipping a fix in a release can be expedited as compared to a completely useless sequence of calls that happen to trigger a crash).
> 
> In cases other than crashes, a regression test is also useful for comparing behavior with other browsers.

Fair enough. I'll see if I can devise a test that is appropriate.

I've looked and didn't see anything like a unit test for AnimationController ... is there one and I've just missed it?

Otherwise, I'll see if an appropriate layout test can be devised, although it may be challenging to try and recreate something with node == 0 as, at present, the existing code doesn't seem to have crashed, suggesting node == 0 is not a scenario that has occured (indeed, in AnimationController::updateAnimations(), there is "ASSERT(renderer()->node()); // FIXME: We do not generate animated content yet." I was actually trying to prototype such "generated content" when I founf the other asserts.

Not to be too verbose, but I'm new to this area of the codebase, so would appreciate any thoughts about what might make a good test.

[Simon Fraser (smfr)]

AnimationController is exercised by tests in LayoutTests/animations and LayoutTests/transitions.

To have a renderer will a null node, you'd need to be animating a pseudoelement (which we did via bug 85253, but that was rolled out).

[W. James MacLean]

(In reply to comment #5)
> AnimationController is exercised by tests in LayoutTests/animations and LayoutTests/transitions.
> 
> To have a renderer will a null node, you'd need to be animating a pseudoelement (which we did via bug 85253, but that was rolled out).

This is similar in a sense to what I was attempting - a RenderObject that provides a transparent & animating highlight to a target node, but has no node itself.

That being said, it seems like a reasonable change to make, if only to get the ASSERTs and the code that follows into a logically consistent state (and a state that might admit the idea of node-less animating ro's). But the challenge becomes that of devising a test.

[Eric Seidel (no email)]

Comment on attachment 153008 [details]
Patch

I agree the code looks wrong.  BUt it's not clear if the ASSERTS are wrong or the code.  We need some sort of test here.

[W. James MacLean]

(In reply to comment #7)
> (From update of attachment 153008 [details])
> I agree the code looks wrong.  BUt it's not clear if the ASSERTS are wrong or the code.  We need some sort of test here.

I'm not experienced enough with this part of the code to say (for sure) which is and isn't correct (except that the particular instance in question is certainly self-contradictory). I was extrapolating from other asserts on the same value. I'll have to defer here to someone with more experience with this code.

[Ahmad Saleem]

AnimationController.cpp got renamed to CSSAnimationController.cpp

Here - https://github.com/WebKit/WebKit/commit/a2eed4f7d11a1257739a9fa80d3a85c5b905c24d

and then got removed here:

https://github.com/WebKit/WebKit/commit/15433766d180265ef8953942e2ec0d23eb7d8f52

@Antoine - do we need anything from this patch?

[Antoine Quint]

We don't, let's close this bug.