Bug 37048

Summary: transitions/cancel-transition.html crashed on Leopard Debug Bot
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: Tools / TestsAssignee: James Robinson <jamesr>
Status: REOPENED ---    
Severity: Normal CC: cmarrin, jamesr, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 28461    
Bug Blocks:    
Attachments:
Description Flags
Patch none

Description Eric Seidel (no email) 2010-04-02 15:30:12 PDT
transitions/cancel-transition.html crashed on Leopard Debug Bot

http://build.webkit.org/results/Leopard%20Intel%20Debug%20(Tests)/r57025%20(12365)/transitions/cancel-transition-stderr.txt
ASSERTION FAILED: newOutlineBox == renderer()->outlineBoundsForRepaint(repaintContainer, 0)
(/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebCore/rendering/RenderLayer.cpp:321 void WebCore::RenderLayer::updateLayerPositions(unsigned int, WebCore::IntPoint*))

http://trac.webkit.org/browser/trunk/LayoutTests/transitions/cancel-transition.html

The ASSERT was added by James Robinson:
http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderLayer.cpp?annotate=blame&rev=57000#L321
Comment 1 Eric Seidel (no email) 2010-04-02 15:30:50 PDT
It's possible this could be related to bug 28461.
Comment 2 Simon Fraser (smfr) 2010-04-02 15:38:09 PDT
Ah, yes, it crashes if accelerated compositing is disabled.
Comment 3 Simon Fraser (smfr) 2010-04-02 15:47:01 PDT
Where "crash" is an assertion. We're computing a repaint rect on some animating transform whose value is time-dependent.
Comment 4 James Robinson 2010-04-02 16:00:26 PDT
If the ASSERT fails it means that the outline box generated using the cached offset differs from the outline box using the slow path (walking up the container hierarchy to generate the offset).  If the offset is time-dependent than this makes sense, the cache offset might be correct at time X but incorrect at time X+1ms.

Should I take out the ASSERT()?  IMHO the cleanest solution is for all time-dependent values in animations to be fixed when painting a single frame.
Comment 5 Simon Fraser (smfr) 2010-04-02 16:14:51 PDT
> Should I take out the ASSERT()?

For now, yes.

> IMHO the cleanest solution is for all time-dependent values in animations to be fixed when painting a single frame.

I think this is the correct solution, yes. Let's keep this bug open to do that (or file a new one).
Comment 6 James Robinson 2010-04-02 16:30:39 PDT
Created attachment 52463 [details]
Patch
Comment 7 Simon Fraser (smfr) 2010-04-02 16:37:18 PDT
Comment on attachment 52463 [details]
Patch

I'd like to see a //FIXME comment there that references this bug.
r=me
Comment 8 James Robinson 2010-04-02 16:52:37 PDT
Committed r57033: <http://trac.webkit.org/changeset/57033>
Comment 9 James Robinson 2010-04-02 16:54:10 PDT
The ASSERT() is gone but I don't feel that the underlying bug is fixed.
Comment 10 Eric Seidel (no email) 2010-04-06 23:46:59 PDT
Attachment 52463 [details] was posted by a committer and has review+, assigning to James Robinson for commit.
Comment 11 Eric Seidel (no email) 2010-05-17 00:34:56 PDT
Comment on attachment 52463 [details]
Patch

Obsoleting patch since this was committed.