CSSValue Immediates: Change CSSValue to an object instead of a pointer

Source/core/animation/animatable/AnimatableUnknownTest.cpp

Index: Source/core/animation/animatable/AnimatableUnknownTest.cpp
diff --git a/Source/core/animation/animatable/AnimatableUnknownTest.cpp b/Source/core/animation/animatable/AnimatableUnknownTest.cpp
index 4ed811d930d3461d3df62dc0239045b0dd6bb3f4..9cf25f427b00584efb251e67fb97f7437f631bad 100644
--- a/Source/core/animation/animatable/AnimatableUnknownTest.cpp
+++ b/Source/core/animation/animatable/AnimatableUnknownTest.cpp
@@ -41,6 +41,10 @@ using namespace blink;
 namespace {
 
 class AnimationAnimatableUnknownTest : public ::testing::Test {
+public:
+    AnimationAnimatableUnknownTest()
+        : cssValue(cssValuePool().createIdentifierValue(CSSValueYellow)),
+        otherCSSValue(cssValuePool().createIdentifierValue(CSSValueOrange)) { };
 protected:
     virtual void SetUp()
     {
@@ -51,9 +55,9 @@ protected:
         otherAnimatableUnknown = AnimatableUnknown::create(otherCSSValue);
     }
 
-    RefPtrWillBePersistent<CSSValue> cssValue;
+    CSSValue cssValue;
     RefPtrWillBePersistent<AnimatableValue> animatableUnknown;
-    RefPtrWillBePersistent<CSSValue> otherCSSValue;
+    CSSValue otherCSSValue;

[haraken, 2015/06/11 02:48:53]

This looks wrong. What you're doing here is:

class A {  // Not garbage collected
  Persistent<CSSValue> cssValue;
};

to:

class A {  // Not garbage collected
  CSSValueObject* m_cssValueObject; 
};

This is unsafe because the m_cssValueObject is not traced. In general:

- it is safe to replace Member<CSSValue> to CSSValue.
- but it is unsafe to replace Persistent<CSSValue> to CSSValue.

As far as I scan the CL, this is the only place where you replaced
Persistent<CSSValue> with CSSValue, so I think we can work around the issue here
instead of inventing some general solution. Specifically, you can do something
like:

class PersistentCSSValue : public GarbageCollected<PersistentCSSValue> {
  DEFINE_INLINE_TRACE()
  {
    visitor->trace(cssValue);
  }
  CSSValue cssValue;
};

class AnimationAnimatableUnknownTest {
  ...;
  PersistentCSSValue persistentCSSValue;
};

[haraken, 2015/06/11 02:48:53]

oilpan-reviews@: The GC plugin must detect this error.

[sashab, 2015/06/11 23:05:34]

In this case I could just remove the test fixture altogether; the values are
only really needed in the last test anyway. Good to know about Persistent
though.

[sof, 2015/06/12 05:10:04]

This is an anonymous namespace, where the GC plugin isn't activated.

[sof, 2015/06/12 10:48:08]

That quirk has now been removed, http://crbug.com/499738

     RefPtrWillBePersistent<AnimatableValue> otherAnimatableUnknown;
 };