Bug 21011 - Need to clean up fix in PropertyWrapperGetter::equals a bit (and add test case)
Summary: Need to clean up fix in PropertyWrapperGetter::equals a bit (and add test case)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Chris Marrin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-22 16:24 PDT by Chris Marrin
Modified: 2008-09-29 14:25 PDT (History)
0 users

See Also:


Attachments
Patch, including LayoutTest file (5.03 KB, patch)
2008-09-22 16:27 PDT, Chris Marrin
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2008-09-22 16:24:19 PDT
The fix in changelist 36703 has one remaining problem. The test:

    if ((!a || !b) && b != a)

will fall through in the case where both a and b are null, causing a crash in the deref on the next line. There is no evidence that it is possible for this to happen, but I think the patch I have submitted is a bit safer. The patch also includes a testcase for the crash.
Comment 1 Chris Marrin 2008-09-22 16:27:39 PDT
Created attachment 23681 [details]
Patch, including LayoutTest file
Comment 2 Eric Seidel (no email) 2008-09-24 15:33:55 PDT
Comment on attachment 23681 [details]
Patch, including LayoutTest file

if (!a && !b || a == b) is redundant.

if (!a && !b)
should be sufficient.

Another way to write this woudl be:

if (!a || !b)
    return (a == b);

but I think the two ifs as you've written it is more clear (After removing the redundancy).
Comment 3 Simon Fraser (smfr) 2008-09-29 14:25:01 PDT
Committed r37076
	M	WebCore/ChangeLog
	M	WebCore/page/animation/CompositeAnimation.cpp
	M	WebCore/page/animation/AnimationBase.cpp
	M	LayoutTests/ChangeLog
	A	LayoutTests/transitions/override-transition-crash-expected.txt
	A	LayoutTests/transitions/override-transition-crash.html
r37076 = ff91302a8b84e30eec65022e68b48301e3dac305 (trunk)