Bug 97955 – Fix compilation of V8DependentRetained and JSDependentRetained.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Sprehn]

Fix compilation of V8DependentRetained and JSDependentRetained.

[Elliott Sprehn]

Created attachment 166337 [details]
Patch

[Kentaro Hara]

Comment on attachment 166337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166337&action=review

> Source/WebCore/ChangeLog:8
> +        Fix bad usage of putDirect and removeDirect from JSDependentRetained and fix

The JSC change looks good.

> Source/WebCore/ChangeLog:9
> +        incorrect assumptions about how weak handles work in V8. This is refactored

The point of the V8 fix is:

- Use m_owner.set() instead of m_owern =.
- Call value->release() in the weak callback.

Am I understanding correctly?

> Source/WebCore/bindings/v8/V8DependentRetained.h:37
> +#include <wtf/text/StringBuilder.h>

Nit: Why is this needed?

[Elliott Sprehn]

Comment on attachment 166337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166337&action=review

>> Source/WebCore/ChangeLog:9
>> +        incorrect assumptions about how weak handles work in V8. This is refactored
> 
> The point of the V8 fix is:
> 
> - Use m_owner.set() instead of m_owern =.
> - Call value->release() in the weak callback.
> 
> Am I understanding correctly?

- Yeah, need to use set() since ScopedPersistent doesn't have overloaded operator=.
- I originally had one weakCallback and abarth suggested using two callbacks, one for the owner and one for the value and asserting about the order,  but that was a wrong thing to do since the order of the callbacks doesn't seem to work as expected when v8 destroys everything (whole page navigate). No code uses this code yet, but if you did you'd get asserts.

>> Source/WebCore/bindings/v8/V8DependentRetained.h:37
>> +#include <wtf/text/StringBuilder.h>
> 
> Nit: Why is this needed?

The code didn't compile because someone else refactored it before my other patch landed that actually uses this, and they made it use StringBuilder (see in the create createPropertyName method in the file). No one noticed it broke because nothing uses this yet.

[Kentaro Hara]

Comment on attachment 166337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166337&action=review

>>> Source/WebCore/ChangeLog:9
>>> +        incorrect assumptions about how weak handles work in V8. This is refactored
>> 
>> The point of the V8 fix is:
>> 
>> - Use m_owner.set() instead of m_owern =.
>> - Call value->release() in the weak callback.
>> 
>> Am I understanding correctly?
> 
> - Yeah, need to use set() since ScopedPersistent doesn't have overloaded operator=.
> - I originally had one weakCallback and abarth suggested using two callbacks, one for the owner and one for the value and asserting about the order,  but that was a wrong thing to do since the order of the callbacks doesn't seem to work as expected when v8 destroys everything (whole page navigate). No code uses this code yet, but if you did you'd get asserts.

Makes sense. Thanks for the clarification!

[WebKit Review Bot]

Comment on attachment 166337 [details]
Patch

Clearing flags on attachment: 166337

Committed r129968: <http://trac.webkit.org/changeset/129968>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.